[PATCH v2 0/4] clk: meson: gxbb-aoclk: Add CEC 32k clock

[PATCH v2 0/4] clk: meson: gxbb-aoclk: Add CEC 32k clock

[Neil Armstrong]

In order to support the standalone CEC Controller on the Amlogic SoCs,
a specific CEC 32K clock must be handled in the AO domain.

The CEC 32K AO Clock is a dual divider with dual counter to provide a more
precise 32768Hz clock for the CEC subsystem from the external xtal.

The AO clocks management registers are spread among the AO register space,
so this patch also adds management of these registers mappings then uses them
for the CEC 32K AO clock management.

This patchset :
 - updates the bindings accordingly
 - switch driver to new bindings
 - adds the CEC 32k clock
 - adds the clock binding entry

The DT Update will be sent in another patchset.

Changes since v1 at [1] :
 - move bindings to parent syscon node and move clkc to subnode
 - switch aoclkc to use regmap only register access
 - introduce aoclk-regmap-gate for this purpose until regmap clocks are generic

[1] https://lkml.kernel.org/r/1499336663-23875-1-git-send-email-narmstrong@baylibre.com

Neil Armstrong (4):
  dt-bindings: clock: amlogic,gxbb-aoclkc: Update bindings
  clk: meson: gxbb-aoclk: Switch to regmap for register access
  dt-bindings: clock: gxbb-aoclk: Add CEC 32k clock
  clk: meson: gxbb-aoclk: Add CEC 32k clock

 .../bindings/clock/amlogic,gxbb-aoclkc.txt         |  22 ++-
 drivers/clk/meson/Makefile                         |   2 +-
 drivers/clk/meson/gxbb-aoclk-32k.c                 | 180 +++++++++++++++++++++
 drivers/clk/meson/gxbb-aoclk-regmap.c              |  46 ++++++
 drivers/clk/meson/gxbb-aoclk.c                     |  65 +++++---
 drivers/clk/meson/gxbb-aoclk.h                     |  42 +++++
 include/dt-bindings/clock/gxbb-aoclkc.h            |   1 +
 7 files changed, 328 insertions(+), 30 deletions(-)
 create mode 100644 drivers/clk/meson/gxbb-aoclk-32k.c
 create mode 100644 drivers/clk/meson/gxbb-aoclk-regmap.c
 create mode 100644 drivers/clk/meson/gxbb-aoclk.h

-- 
1.9.1

[PATCH v2 1/4] dt-bindings: clock: amlogic,gxbb-aoclkc: Update bindings

[Neil Armstrong]

On the first revision of the bindings, only the gates + resets were known
in the AO Clock HW, but more registers used to configures AO clock are known
to be spread among the AO register space.
This patch adds a parent node for the entire system control zone for the AO
domain then moves the clock controller as a subnode of the system control
node.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 .../bindings/clock/amlogic,gxbb-aoclkc.txt         | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/amlogic,gxbb-aoclkc.txt b/Documentation/devicetree/bindings/clock/amlogic,gxbb-aoclkc.txt
index a55d31b..64884ed 100644
--- a/Documentation/devicetree/bindings/clock/amlogic,gxbb-aoclkc.txt
+++ b/Documentation/devicetree/bindings/clock/amlogic,gxbb-aoclkc.txt
@@ -5,9 +5,11 @@ controllers within the Always-On part of the SoC.
 
 Required Properties:
 
-- compatible: should be "amlogic,gxbb-aoclkc"
-- reg: physical base address of the clock controller and length of memory
-       mapped region.
+- compatible: value should be different for each SoC family as :
+	- GXBB (S905) : "amlogic,meson-gxbb-aoclkc"
+	- GXL (S905X, S905D) : "amlogic,meson-gxl-aoclkc"
+	- GXM (S912) : "amlogic,meson-gxm-aoclkc"
+	followed by the common "amlogic,meson-gx-aoclkc"
 
 - #clock-cells: should be 1.
 
@@ -23,14 +25,22 @@ to specify the reset which they consume. All available resets are defined as
 preprocessor macros in the dt-bindings/reset/gxbb-aoclkc.h header and can be
 used in device tree sources.
 
+Parent node should have the following properties :
+- compatible: "amlogic,meson-gx-ao-sysctrl", "syscon", "simple-mfd"
+- reg: base address and size of the AO system control register space.
+
 Example: AO Clock controller node:
 
-	clkc_AO: clock-controller@040 {
-		compatible = "amlogic,gxbb-aoclkc";
-		reg = <0x0 0x040 0x0 0x4>;
+ao_sysctrl: sys-ctrl@0 {
+	compatible = "amlogic,meson-gx-ao-sysctrl", "syscon", "simple-mfd";
+	reg =  <0x0 0x0 0x0 0x100>;
+
+	clkc_AO: clock-controller {
+		compatible = "amlogic,meson-gxbb-aoclkc", "amlogic,meson-gx-aoclkc";
 		#clock-cells = <1>;
 		#reset-cells = <1>;
 	};
+};
 
 Example: UART controller node that consumes the clock and reset generated
   by the clock controller:
-- 
1.9.1

[PATCH v2 2/4] clk: meson: gxbb-aoclk: Switch to regmap for register access

[Neil Armstrong]

Switch the aoclk driver to use the new bindings and switch all the
registers access to regmap only.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/clk/meson/Makefile            |  2 +-
 drivers/clk/meson/gxbb-aoclk-regmap.c | 46 +++++++++++++++++++++++++++++++++++
 drivers/clk/meson/gxbb-aoclk.c        | 44 ++++++++++++++++-----------------
 drivers/clk/meson/gxbb-aoclk.h        | 26 ++++++++++++++++++++
 4 files changed, 95 insertions(+), 23 deletions(-)
 create mode 100644 drivers/clk/meson/gxbb-aoclk-regmap.c
 create mode 100644 drivers/clk/meson/gxbb-aoclk.h

diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
index 83b6d9d..de65427 100644
--- a/drivers/clk/meson/Makefile
+++ b/drivers/clk/meson/Makefile
@@ -4,4 +4,4 @@
 
 obj-$(CONFIG_COMMON_CLK_AMLOGIC) += clk-pll.o clk-cpu.o clk-mpll.o clk-audio-divider.o
 obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o
-obj-$(CONFIG_COMMON_CLK_GXBB)	 += gxbb.o gxbb-aoclk.o
+obj-$(CONFIG_COMMON_CLK_GXBB)	 += gxbb.o gxbb-aoclk.o gxbb-aoclk-regmap.o
diff --git a/drivers/clk/meson/gxbb-aoclk-regmap.c b/drivers/clk/meson/gxbb-aoclk-regmap.c
new file mode 100644
index 0000000..2515fbf
--- /dev/null
+++ b/drivers/clk/meson/gxbb-aoclk-regmap.c
@@ -0,0 +1,46 @@
+/*
+ * Copyright (c) 2017 BayLibre, SAS.
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/bitfield.h>
+#include <linux/regmap.h>
+#include "gxbb-aoclk.h"
+
+static int aoclk_gate_regmap_enable(struct clk_hw *hw)
+{
+	struct aoclk_gate_regmap *gate = to_aoclk_gate_regmap(hw);
+
+	return regmap_update_bits(gate->regmap, AO_RTI_GEN_CNTL_REG0,
+				  BIT(gate->bit_idx), BIT(gate->bit_idx));
+}
+
+static void aoclk_gate_regmap_disable(struct clk_hw *hw)
+{
+	struct aoclk_gate_regmap *gate = to_aoclk_gate_regmap(hw);
+
+	regmap_update_bits(gate->regmap, AO_RTI_GEN_CNTL_REG0,
+			   BIT(gate->bit_idx), 0);
+}
+
+static int aoclk_gate_regmap_is_enabled(struct clk_hw *hw)
+{
+	struct aoclk_gate_regmap *gate = to_aoclk_gate_regmap(hw);
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(gate->regmap, AO_RTI_GEN_CNTL_REG0, &val);
+	if (ret)
+		return ret;
+
+	return (val & BIT(gate->bit_idx)) != 0;
+}
+
+const struct clk_ops meson_aoclk_gate_regmap_ops = {
+	.enable = aoclk_gate_regmap_enable,
+	.disable = aoclk_gate_regmap_disable,
+	.is_enabled = aoclk_gate_regmap_is_enabled,
+};
diff --git a/drivers/clk/meson/gxbb-aoclk.c b/drivers/clk/meson/gxbb-aoclk.c
index b45c5fb..f61506c 100644
--- a/drivers/clk/meson/gxbb-aoclk.c
+++ b/drivers/clk/meson/gxbb-aoclk.c
@@ -56,16 +56,19 @@
 #include <linux/of_address.h>
 #include <linux/platform_device.h>
 #include <linux/reset-controller.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
 #include <linux/init.h>
 #include <dt-bindings/clock/gxbb-aoclkc.h>
 #include <dt-bindings/reset/gxbb-aoclkc.h>
+#include "gxbb-aoclk.h"
 
 static DEFINE_SPINLOCK(gxbb_aoclk_lock);
 
 struct gxbb_aoclk_reset_controller {
 	struct reset_controller_dev reset;
 	unsigned int *data;
-	void __iomem *base;
+	struct regmap *regmap;
 };
 
 static int gxbb_aoclk_do_reset(struct reset_controller_dev *rcdev,
@@ -74,9 +77,8 @@ static int gxbb_aoclk_do_reset(struct reset_controller_dev *rcdev,
 	struct gxbb_aoclk_reset_controller *reset =
 		container_of(rcdev, struct gxbb_aoclk_reset_controller, reset);
 
-	writel(BIT(reset->data[id]), reset->base);
-
-	return 0;
+	return regmap_write(reset->regmap, AO_RTI_GEN_CNTL_REG0,
+			    BIT(reset->data[id]));
 }
 
 static const struct reset_control_ops gxbb_aoclk_reset_ops = {
@@ -84,13 +86,12 @@ static int gxbb_aoclk_do_reset(struct reset_controller_dev *rcdev,
 };
 
 #define GXBB_AO_GATE(_name, _bit)					\
-static struct clk_gate _name##_ao = {					\
-	.reg = (void __iomem *)0,					\
+static struct aoclk_gate_regmap _name##_ao = {				\
 	.bit_idx = (_bit),						\
 	.lock = &gxbb_aoclk_lock,					\
 	.hw.init = &(struct clk_init_data) {				\
 		.name = #_name "_ao",					\
-		.ops = &clk_gate_ops,					\
+		.ops = &meson_aoclk_gate_regmap_ops,			\
 		.parent_names = (const char *[]){ "clk81" },		\
 		.num_parents = 1,					\
 		.flags = (CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED),	\
@@ -113,7 +114,7 @@ static int gxbb_aoclk_do_reset(struct reset_controller_dev *rcdev,
 	[RESET_AO_IR_BLASTER] = 23,
 };
 
-static struct clk_gate *gxbb_aoclk_gate[] = {
+static struct aoclk_gate_regmap *gxbb_aoclk_gate[] = {
 	[CLKID_AO_REMOTE] = &remote_ao,
 	[CLKID_AO_I2C_MASTER] = &i2c_master_ao,
 	[CLKID_AO_I2C_SLAVE] = &i2c_slave_ao,
@@ -136,24 +137,23 @@ static int gxbb_aoclk_do_reset(struct reset_controller_dev *rcdev,
 
 static int gxbb_aoclkc_probe(struct platform_device *pdev)
 {
-	struct resource *res;
-	void __iomem *base;
-	int ret, clkid;
-	struct device *dev = &pdev->dev;
 	struct gxbb_aoclk_reset_controller *rstc;
+	struct device *dev = &pdev->dev;
+	struct regmap *regmap;
+	int ret, clkid;
 
 	rstc = devm_kzalloc(dev, sizeof(*rstc), GFP_KERNEL);
 	if (!rstc)
 		return -ENOMEM;
 
-	/* Generic clocks */
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	base = devm_ioremap_resource(dev, res);
-	if (IS_ERR(base))
-		return PTR_ERR(base);
+	regmap = syscon_node_to_regmap(of_get_parent(dev->of_node));
+	if (IS_ERR(regmap)) {
+		dev_err(dev, "failed to get regmap\n");
+		return -ENODEV;
+	}
 
 	/* Reset Controller */
-	rstc->base = base;
+	rstc->regmap = regmap;
 	rstc->data = gxbb_aoclk_reset;
 	rstc->reset.ops = &gxbb_aoclk_reset_ops;
 	rstc->reset.nr_resets = ARRAY_SIZE(gxbb_aoclk_reset);
@@ -161,10 +161,10 @@ static int gxbb_aoclkc_probe(struct platform_device *pdev)
 	ret = devm_reset_controller_register(dev, &rstc->reset);
 
 	/*
-	 * Populate base address and register all clks
+	 * Populate regmap and register all clks
 	 */
-	for (clkid = 0; clkid < gxbb_aoclk_onecell_data.num; clkid++) {
-		gxbb_aoclk_gate[clkid]->reg = base;
+	for (clkid = 0; clkid < ARRAY_SIZE(gxbb_aoclk_gate); clkid++) {
+		gxbb_aoclk_gate[clkid]->regmap = regmap;
 
 		ret = devm_clk_hw_register(dev,
 					gxbb_aoclk_onecell_data.hws[clkid]);
@@ -177,7 +177,7 @@ static int gxbb_aoclkc_probe(struct platform_device *pdev)
 }
 
 static const struct of_device_id gxbb_aoclkc_match_table[] = {
-	{ .compatible = "amlogic,gxbb-aoclkc" },
+	{ .compatible = "amlogic,meson-gx-aoclkc" },
 	{ }
 };
 
diff --git a/drivers/clk/meson/gxbb-aoclk.h b/drivers/clk/meson/gxbb-aoclk.h
new file mode 100644
index 0000000..2e26108
--- /dev/null
+++ b/drivers/clk/meson/gxbb-aoclk.h
@@ -0,0 +1,26 @@
+/*
+ * Copyright (c) 2017 BayLibre, SAS
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+#ifndef __GXBB_AOCLKC_H
+#define __GXBB_AOCLKC_H
+
+/* AO Configuration Clock registers offsets */
+#define AO_RTI_GEN_CNTL_REG0	0x40
+
+struct aoclk_gate_regmap {
+	struct clk_hw hw;
+	unsigned bit_idx;
+	struct regmap *regmap;
+	spinlock_t *lock;
+};
+
+#define to_aoclk_gate_regmap(_hw) \
+	container_of(_hw, struct aoclk_gate_regmap, hw)
+
+extern const struct clk_ops meson_aoclk_gate_regmap_ops;
+
+#endif /* __GXBB_AOCLKC_H */
-- 
1.9.1

[PATCH v2 3/4] dt-bindings: clock: gxbb-aoclk: Add CEC 32k clock

[Neil Armstrong]

This patchadds the clock binding entry for the CEC 32K AO Clock.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 include/dt-bindings/clock/gxbb-aoclkc.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/dt-bindings/clock/gxbb-aoclkc.h b/include/dt-bindings/clock/gxbb-aoclkc.h
index 3175148..9d15e22 100644
--- a/include/dt-bindings/clock/gxbb-aoclkc.h
+++ b/include/dt-bindings/clock/gxbb-aoclkc.h
@@ -62,5 +62,6 @@
 #define CLKID_AO_UART1		3
 #define CLKID_AO_UART2		4
 #define CLKID_AO_IR_BLASTER	5
+#define CLKID_AO_CEC_32K	6
 
 #endif
-- 
1.9.1

[PATCH v2 4/4] clk: meson: gxbb-aoclk: Add CEC 32k clock

[Neil Armstrong]

The CEC 32K AO Clock is a dual divider with dual counter to provide a more
precise 32768Hz clock for the CEC subsystem from the external xtal.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/clk/meson/Makefile         |   2 +-
 drivers/clk/meson/gxbb-aoclk-32k.c | 180 +++++++++++++++++++++++++++++++++++++
 drivers/clk/meson/gxbb-aoclk.c     |  21 ++++-
 drivers/clk/meson/gxbb-aoclk.h     |  16 ++++
 4 files changed, 217 insertions(+), 2 deletions(-)
 create mode 100644 drivers/clk/meson/gxbb-aoclk-32k.c

diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
index de65427..b139d41 100644
--- a/drivers/clk/meson/Makefile
+++ b/drivers/clk/meson/Makefile
@@ -4,4 +4,4 @@
 
 obj-$(CONFIG_COMMON_CLK_AMLOGIC) += clk-pll.o clk-cpu.o clk-mpll.o clk-audio-divider.o
 obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o
-obj-$(CONFIG_COMMON_CLK_GXBB)	 += gxbb.o gxbb-aoclk.o gxbb-aoclk-regmap.o
+obj-$(CONFIG_COMMON_CLK_GXBB)	 += gxbb.o gxbb-aoclk.o gxbb-aoclk-regmap.o gxbb-aoclk-32k.o
diff --git a/drivers/clk/meson/gxbb-aoclk-32k.c b/drivers/clk/meson/gxbb-aoclk-32k.c
new file mode 100644
index 0000000..497cd09
--- /dev/null
+++ b/drivers/clk/meson/gxbb-aoclk-32k.c
@@ -0,0 +1,180 @@
+/*
+ * Copyright (c) 2017 BayLibre, SAS.
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/bitfield.h>
+#include <linux/regmap.h>
+#include "gxbb-aoclk.h"
+
+/*
+ * The AO Domain embeds a dual/divider to generate a more precise
+ * 32,768KHz clock for low-power suspend mode and CEC.
+ */
+
+#define CLK_CNTL0_N1_MASK	GENMASK(11, 0)
+#define CLK_CNTL0_N2_MASK	GENMASK(23, 12)
+#define CLK_CNTL0_DUALDIV_EN	BIT(28)
+#define CLK_CNTL0_OUT_GATE_EN	BIT(30)
+#define CLK_CNTL0_IN_GATE_EN	BIT(31)
+
+#define CLK_CNTL1_M1_MASK	GENMASK(11, 0)
+#define CLK_CNTL1_M2_MASK	GENMASK(23, 12)
+#define CLK_CNTL1_BYPASS_EN	BIT(24)
+#define CLK_CNTL1_SELECT_OSC	BIT(27)
+
+#define PWR_CNTL_ALT_32K_SEL	GENMASK(13, 10)
+
+struct cec_32k_freq_table {
+	unsigned long parent_rate;
+	unsigned long target_rate;
+	bool dualdiv;
+	unsigned int n1;
+	unsigned int n2;
+	unsigned int m1;
+	unsigned int m2;
+};
+
+static const struct cec_32k_freq_table aoclk_cec_32k_table[] = {
+	[0] = {
+		.parent_rate = 24000000,
+		.target_rate = 32768,
+		.dualdiv = true,
+		.n1 = 733,
+		.n2 = 732,
+		.m1 = 8,
+		.m2 = 11,
+	},
+};
+
+/*
+ * If CLK_CNTL0_DUALDIV_EN == 0
+ *  - will use N1 divider only
+ * If CLK_CNTL0_DUALDIV_EN == 1
+ *  - hold M1 cycles of N1 divider then changes to N2
+ *  - hold M2 cycles of N2 divider then changes to N1
+ * Then we can get more accurate division.
+ */
+static unsigned long aoclk_cec_32k_recalc_rate(struct clk_hw *hw,
+					       unsigned long parent_rate)
+{
+	struct aoclk_cec_32k *cec_32k = to_aoclk_cec_32k(hw);
+	unsigned long n1;
+	u32 reg0, reg1;
+
+	regmap_read(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL0, &reg0);
+	regmap_read(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL1, &reg1);
+
+	if (reg1 & CLK_CNTL1_BYPASS_EN)
+		return parent_rate;
+
+	if (reg0 & CLK_CNTL0_DUALDIV_EN) {
+		unsigned long n2, m1, m2, f1, f2, p1, p2;
+
+		n1 = FIELD_GET(CLK_CNTL0_N1_MASK, reg0) + 1;
+		n2 = FIELD_GET(CLK_CNTL0_N2_MASK, reg0) + 1;
+
+		m1 = FIELD_GET(CLK_CNTL1_M1_MASK, reg1) + 1;
+		m2 = FIELD_GET(CLK_CNTL1_M2_MASK, reg1) + 1;
+
+		f1 = DIV_ROUND_CLOSEST(parent_rate, n1);
+		f2 = DIV_ROUND_CLOSEST(parent_rate, n2);
+
+		p1 = DIV_ROUND_CLOSEST(100000000 * m1, f1 * (m1 + m2));
+		p2 = DIV_ROUND_CLOSEST(100000000 * m2, f2 * (m1 + m2));
+
+		return DIV_ROUND_UP(100000000, p1 + p2);
+	}
+
+	n1 = FIELD_GET(CLK_CNTL0_N1_MASK, reg0) + 1;
+
+	return DIV_ROUND_CLOSEST(parent_rate, n1);
+}
+
+static const struct cec_32k_freq_table *find_cec_32k_freq(unsigned long rate,
+							  unsigned long prate)
+{
+	int i;
+
+	for (i = 0 ; i < ARRAY_SIZE(aoclk_cec_32k_table) ; ++i)
+		if (aoclk_cec_32k_table[i].parent_rate == prate &&
+		    aoclk_cec_32k_table[i].target_rate == rate)
+			return &aoclk_cec_32k_table[i];
+
+	return NULL;
+}
+
+static long aoclk_cec_32k_round_rate(struct clk_hw *hw, unsigned long rate,
+				     unsigned long *prate)
+{
+	const struct cec_32k_freq_table *freq = find_cec_32k_freq(rate,
+								  *prate);
+
+	/* If invalid return first one */
+	if (!freq)
+		return freq[0].target_rate;
+
+	return freq->target_rate;
+}
+
+static int aoclk_cec_32k_set_rate(struct clk_hw *hw, unsigned long rate,
+				  unsigned long parent_rate)
+{
+	const struct cec_32k_freq_table *freq = find_cec_32k_freq(rate,
+								  parent_rate);
+	struct aoclk_cec_32k *cec_32k = to_aoclk_cec_32k(hw);
+	u32 reg = 0;
+
+	if (!freq)
+		return -EINVAL;
+
+	/* Disable clock */
+	regmap_update_bits(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL0,
+			   CLK_CNTL0_IN_GATE_EN | CLK_CNTL0_OUT_GATE_EN, 0);
+
+	if (freq->dualdiv)
+		reg = CLK_CNTL0_DUALDIV_EN |
+		      FIELD_PREP(CLK_CNTL0_N1_MASK, freq->n1 - 1) |
+		      FIELD_PREP(CLK_CNTL0_N2_MASK, freq->n2 - 1);
+	else
+		reg = FIELD_PREP(CLK_CNTL0_N1_MASK, freq->n1 - 1);
+
+	regmap_write(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL0, reg);
+
+	if (freq->dualdiv)
+		reg = FIELD_PREP(CLK_CNTL1_M1_MASK, freq->m1 - 1) |
+		      FIELD_PREP(CLK_CNTL1_M2_MASK, freq->m2 - 1);
+	else
+		reg = FIELD_PREP(CLK_CNTL1_M1_MASK, freq->m1 - 1);
+
+	regmap_write(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL1, reg);
+
+	/* Enable clock */
+	regmap_update_bits(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL0,
+			   CLK_CNTL0_IN_GATE_EN, CLK_CNTL0_IN_GATE_EN);
+
+	udelay(200);
+
+	regmap_update_bits(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL0,
+			   CLK_CNTL0_OUT_GATE_EN, CLK_CNTL0_OUT_GATE_EN);
+
+	regmap_update_bits(cec_32k->regmap, AO_CRT_CLK_CNTL1,
+			   CLK_CNTL1_SELECT_OSC, CLK_CNTL1_SELECT_OSC);
+
+	/* Select 32k from XTAL */
+	regmap_update_bits(cec_32k->regmap,
+			  AO_RTI_PWR_CNTL_REG0,
+			  PWR_CNTL_ALT_32K_SEL,
+			  FIELD_PREP(PWR_CNTL_ALT_32K_SEL, 4));
+
+	return 0;
+}
+
+const struct clk_ops meson_aoclk_cec_32k_ops = {
+	.recalc_rate = aoclk_cec_32k_recalc_rate,
+	.round_rate = aoclk_cec_32k_round_rate,
+	.set_rate = aoclk_cec_32k_set_rate,
+};
diff --git a/drivers/clk/meson/gxbb-aoclk.c b/drivers/clk/meson/gxbb-aoclk.c
index f61506c..6c161e0 100644
--- a/drivers/clk/meson/gxbb-aoclk.c
+++ b/drivers/clk/meson/gxbb-aoclk.c
@@ -59,6 +59,7 @@
 #include <linux/mfd/syscon.h>
 #include <linux/regmap.h>
 #include <linux/init.h>
+#include <linux/delay.h>
 #include <dt-bindings/clock/gxbb-aoclkc.h>
 #include <dt-bindings/reset/gxbb-aoclkc.h>
 #include "gxbb-aoclk.h"
@@ -105,6 +106,17 @@ static int gxbb_aoclk_do_reset(struct reset_controller_dev *rcdev,
 GXBB_AO_GATE(uart2, 5);
 GXBB_AO_GATE(ir_blaster, 6);
 
+static struct aoclk_cec_32k cec_32k_ao = {
+	.lock = &gxbb_aoclk_lock,
+	.hw.init = &(struct clk_init_data) {
+		.name = "cec_32k_ao",
+		.ops = &meson_aoclk_cec_32k_ops,
+		.parent_names = (const char *[]){ "xtal" },
+		.num_parents = 1,
+		.flags = CLK_IGNORE_UNUSED,
+	},
+};
+
 static unsigned int gxbb_aoclk_reset[] = {
 	[RESET_AO_REMOTE] = 16,
 	[RESET_AO_I2C_MASTER] = 18,
@@ -131,8 +143,9 @@ static int gxbb_aoclk_do_reset(struct reset_controller_dev *rcdev,
 		[CLKID_AO_UART1] = &uart1_ao.hw,
 		[CLKID_AO_UART2] = &uart2_ao.hw,
 		[CLKID_AO_IR_BLASTER] = &ir_blaster_ao.hw,
+		[CLKID_AO_CEC_32K] = &cec_32k_ao.hw,
 	},
-	.num = ARRAY_SIZE(gxbb_aoclk_gate),
+	.num = 7,
 };
 
 static int gxbb_aoclkc_probe(struct platform_device *pdev)
@@ -172,6 +185,12 @@ static int gxbb_aoclkc_probe(struct platform_device *pdev)
 			return ret;
 	}
 
+	/* Specific clocks */
+	cec_32k_ao.regmap = regmap;
+	ret = devm_clk_hw_register(dev, &cec_32k_ao.hw);
+	if (ret)
+		return ret;
+
 	return of_clk_add_hw_provider(dev->of_node, of_clk_hw_onecell_get,
 			&gxbb_aoclk_onecell_data);
 }
diff --git a/drivers/clk/meson/gxbb-aoclk.h b/drivers/clk/meson/gxbb-aoclk.h
index 2e26108..e8604c8 100644
--- a/drivers/clk/meson/gxbb-aoclk.h
+++ b/drivers/clk/meson/gxbb-aoclk.h
@@ -9,7 +9,13 @@
 #define __GXBB_AOCLKC_H
 
 /* AO Configuration Clock registers offsets */
+#define AO_RTI_PWR_CNTL_REG1	0x0c
+#define AO_RTI_PWR_CNTL_REG0	0x10
 #define AO_RTI_GEN_CNTL_REG0	0x40
+#define AO_OSCIN_CNTL		0x58
+#define AO_CRT_CLK_CNTL1	0x68
+#define AO_RTC_ALT_CLK_CNTL0	0x94
+#define AO_RTC_ALT_CLK_CNTL1	0x98
 
 struct aoclk_gate_regmap {
 	struct clk_hw hw;
@@ -23,4 +29,14 @@ struct aoclk_gate_regmap {
 
 extern const struct clk_ops meson_aoclk_gate_regmap_ops;
 
+struct aoclk_cec_32k {
+	struct clk_hw hw;
+	struct regmap *regmap;
+	spinlock_t *lock;
+};
+
+#define to_aoclk_cec_32k(_hw) container_of(_hw, struct aoclk_cec_32k, hw)
+
+extern const struct clk_ops meson_aoclk_cec_32k_ops;
+
 #endif /* __GXBB_AOCLKC_H */
-- 
1.9.1

Re: [PATCH v2 4/4] clk: meson: gxbb-aoclk: Add CEC 32k clock

[Martin Blumenstingl]

Hi Neil,

thanks for these patches, CEC support is another good step!

On Fri, Jul 28, 2017 at 11:53 AM, Neil Armstrong
<narmstrong@baylibre.com> wrote:
> The CEC 32K AO Clock is a dual divider with dual counter to provide a more
> precise 32768Hz clock for the CEC subsystem from the external xtal.
>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/clk/meson/Makefile         |   2 +-
>  drivers/clk/meson/gxbb-aoclk-32k.c | 180 +++++++++++++++++++++++++++++++++++++
>  drivers/clk/meson/gxbb-aoclk.c     |  21 ++++-
>  drivers/clk/meson/gxbb-aoclk.h     |  16 ++++
>  4 files changed, 217 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/clk/meson/gxbb-aoclk-32k.c
>
> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
> index de65427..b139d41 100644
> --- a/drivers/clk/meson/Makefile
> +++ b/drivers/clk/meson/Makefile
> @@ -4,4 +4,4 @@
>
>  obj-$(CONFIG_COMMON_CLK_AMLOGIC) += clk-pll.o clk-cpu.o clk-mpll.o clk-audio-divider.o
>  obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o
> -obj-$(CONFIG_COMMON_CLK_GXBB)   += gxbb.o gxbb-aoclk.o gxbb-aoclk-regmap.o
> +obj-$(CONFIG_COMMON_CLK_GXBB)   += gxbb.o gxbb-aoclk.o gxbb-aoclk-regmap.o gxbb-aoclk-32k.o
> diff --git a/drivers/clk/meson/gxbb-aoclk-32k.c b/drivers/clk/meson/gxbb-aoclk-32k.c
> new file mode 100644
> index 0000000..497cd09
> --- /dev/null
> +++ b/drivers/clk/meson/gxbb-aoclk-32k.c
> @@ -0,0 +1,180 @@
> +/*
> + * Copyright (c) 2017 BayLibre, SAS.
> + * Author: Neil Armstrong <narmstrong@baylibre.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/bitfield.h>
> +#include <linux/regmap.h>
> +#include "gxbb-aoclk.h"
> +
> +/*
> + * The AO Domain embeds a dual/divider to generate a more precise
> + * 32,768KHz clock for low-power suspend mode and CEC.
> + */
> +
> +#define CLK_CNTL0_N1_MASK      GENMASK(11, 0)
> +#define CLK_CNTL0_N2_MASK      GENMASK(23, 12)
> +#define CLK_CNTL0_DUALDIV_EN   BIT(28)
> +#define CLK_CNTL0_OUT_GATE_EN  BIT(30)
> +#define CLK_CNTL0_IN_GATE_EN   BIT(31)
> +
> +#define CLK_CNTL1_M1_MASK      GENMASK(11, 0)
> +#define CLK_CNTL1_M2_MASK      GENMASK(23, 12)
> +#define CLK_CNTL1_BYPASS_EN    BIT(24)
> +#define CLK_CNTL1_SELECT_OSC   BIT(27)
> +
> +#define PWR_CNTL_ALT_32K_SEL   GENMASK(13, 10)
> +
> +struct cec_32k_freq_table {
> +       unsigned long parent_rate;
> +       unsigned long target_rate;
> +       bool dualdiv;
> +       unsigned int n1;
> +       unsigned int n2;
> +       unsigned int m1;
> +       unsigned int m2;
> +};
> +
> +static const struct cec_32k_freq_table aoclk_cec_32k_table[] = {
> +       [0] = {
> +               .parent_rate = 24000000,
shouldn't you add a parent (XTAL) to this new clock so you could get
rid of this?

> +               .target_rate = 32768,
> +               .dualdiv = true,
> +               .n1 = 733,
> +               .n2 = 732,
> +               .m1 = 8,
> +               .m2 = 11,
> +       },
> +};
> +
> +/*
> + * If CLK_CNTL0_DUALDIV_EN == 0
> + *  - will use N1 divider only
> + * If CLK_CNTL0_DUALDIV_EN == 1
> + *  - hold M1 cycles of N1 divider then changes to N2
> + *  - hold M2 cycles of N2 divider then changes to N1
> + * Then we can get more accurate division.
> + */
> +static unsigned long aoclk_cec_32k_recalc_rate(struct clk_hw *hw,
> +                                              unsigned long parent_rate)
> +{
> +       struct aoclk_cec_32k *cec_32k = to_aoclk_cec_32k(hw);
> +       unsigned long n1;
> +       u32 reg0, reg1;
> +
> +       regmap_read(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL0, &reg0);
> +       regmap_read(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL1, &reg1);
> +
> +       if (reg1 & CLK_CNTL1_BYPASS_EN)
> +               return parent_rate;
> +
> +       if (reg0 & CLK_CNTL0_DUALDIV_EN) {
> +               unsigned long n2, m1, m2, f1, f2, p1, p2;
> +
> +               n1 = FIELD_GET(CLK_CNTL0_N1_MASK, reg0) + 1;
> +               n2 = FIELD_GET(CLK_CNTL0_N2_MASK, reg0) + 1;
> +
> +               m1 = FIELD_GET(CLK_CNTL1_M1_MASK, reg1) + 1;
> +               m2 = FIELD_GET(CLK_CNTL1_M2_MASK, reg1) + 1;
> +
> +               f1 = DIV_ROUND_CLOSEST(parent_rate, n1);
> +               f2 = DIV_ROUND_CLOSEST(parent_rate, n2);
> +
> +               p1 = DIV_ROUND_CLOSEST(100000000 * m1, f1 * (m1 + m2));
> +               p2 = DIV_ROUND_CLOSEST(100000000 * m2, f2 * (m1 + m2));
> +
> +               return DIV_ROUND_UP(100000000, p1 + p2);
> +       }
> +
> +       n1 = FIELD_GET(CLK_CNTL0_N1_MASK, reg0) + 1;
> +
> +       return DIV_ROUND_CLOSEST(parent_rate, n1);
> +}
> +
> +static const struct cec_32k_freq_table *find_cec_32k_freq(unsigned long rate,
> +                                                         unsigned long prate)
> +{
> +       int i;
> +
> +       for (i = 0 ; i < ARRAY_SIZE(aoclk_cec_32k_table) ; ++i)
> +               if (aoclk_cec_32k_table[i].parent_rate == prate &&
> +                   aoclk_cec_32k_table[i].target_rate == rate)
> +                       return &aoclk_cec_32k_table[i];
> +
> +       return NULL;
> +}
> +
> +static long aoclk_cec_32k_round_rate(struct clk_hw *hw, unsigned long rate,
> +                                    unsigned long *prate)
> +{
> +       const struct cec_32k_freq_table *freq = find_cec_32k_freq(rate,
> +                                                                 *prate);
> +
> +       /* If invalid return first one */
> +       if (!freq)
> +               return freq[0].target_rate;
> +
> +       return freq->target_rate;
> +}
> +
> +static int aoclk_cec_32k_set_rate(struct clk_hw *hw, unsigned long rate,
> +                                 unsigned long parent_rate)
> +{
> +       const struct cec_32k_freq_table *freq = find_cec_32k_freq(rate,
> +                                                                 parent_rate);
> +       struct aoclk_cec_32k *cec_32k = to_aoclk_cec_32k(hw);
> +       u32 reg = 0;
> +
> +       if (!freq)
> +               return -EINVAL;
> +
> +       /* Disable clock */
> +       regmap_update_bits(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL0,
> +                          CLK_CNTL0_IN_GATE_EN | CLK_CNTL0_OUT_GATE_EN, 0);
> +
> +       if (freq->dualdiv)
> +               reg = CLK_CNTL0_DUALDIV_EN |
> +                     FIELD_PREP(CLK_CNTL0_N1_MASK, freq->n1 - 1) |
> +                     FIELD_PREP(CLK_CNTL0_N2_MASK, freq->n2 - 1);
> +       else
> +               reg = FIELD_PREP(CLK_CNTL0_N1_MASK, freq->n1 - 1);
> +
> +       regmap_write(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL0, reg);
> +
> +       if (freq->dualdiv)
> +               reg = FIELD_PREP(CLK_CNTL1_M1_MASK, freq->m1 - 1) |
> +                     FIELD_PREP(CLK_CNTL1_M2_MASK, freq->m2 - 1);
> +       else
> +               reg = FIELD_PREP(CLK_CNTL1_M1_MASK, freq->m1 - 1);
> +
> +       regmap_write(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL1, reg);
> +
> +       /* Enable clock */
> +       regmap_update_bits(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL0,
> +                          CLK_CNTL0_IN_GATE_EN, CLK_CNTL0_IN_GATE_EN);
based on how I understand your code the *input* clock needs to be
gated during rate change, so (in my opinion) it's fine to toggle it
here.

> +
> +       udelay(200);
> +
> +       regmap_update_bits(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL0,
> +                          CLK_CNTL0_OUT_GATE_EN, CLK_CNTL0_OUT_GATE_EN);
however, should we make the output clock configurable by implementing
clk_ops.enable and clk_ops.disable?

> +
> +       regmap_update_bits(cec_32k->regmap, AO_CRT_CLK_CNTL1,
> +                          CLK_CNTL1_SELECT_OSC, CLK_CNTL1_SELECT_OSC);
> +
> +       /* Select 32k from XTAL */
> +       regmap_update_bits(cec_32k->regmap,
> +                         AO_RTI_PWR_CNTL_REG0,
> +                         PWR_CNTL_ALT_32K_SEL,
> +                         FIELD_PREP(PWR_CNTL_ALT_32K_SEL, 4));
is it correct to configure the muxes after enabling the clock output?
also do you know about the other parents of PWR_CNTL_ALT_32K_SEL?

> +
> +       return 0;
> +}
> +
> +const struct clk_ops meson_aoclk_cec_32k_ops = {
> +       .recalc_rate = aoclk_cec_32k_recalc_rate,
> +       .round_rate = aoclk_cec_32k_round_rate,
> +       .set_rate = aoclk_cec_32k_set_rate,
> +};
> diff --git a/drivers/clk/meson/gxbb-aoclk.c b/drivers/clk/meson/gxbb-aoclk.c
> index f61506c..6c161e0 100644
> --- a/drivers/clk/meson/gxbb-aoclk.c
> +++ b/drivers/clk/meson/gxbb-aoclk.c
> @@ -59,6 +59,7 @@
>  #include <linux/mfd/syscon.h>
>  #include <linux/regmap.h>
>  #include <linux/init.h>
> +#include <linux/delay.h>
>  #include <dt-bindings/clock/gxbb-aoclkc.h>
>  #include <dt-bindings/reset/gxbb-aoclkc.h>
>  #include "gxbb-aoclk.h"
> @@ -105,6 +106,17 @@ static int gxbb_aoclk_do_reset(struct reset_controller_dev *rcdev,
>  GXBB_AO_GATE(uart2, 5);
>  GXBB_AO_GATE(ir_blaster, 6);
>
> +static struct aoclk_cec_32k cec_32k_ao = {
> +       .lock = &gxbb_aoclk_lock,
> +       .hw.init = &(struct clk_init_data) {
> +               .name = "cec_32k_ao",
> +               .ops = &meson_aoclk_cec_32k_ops,
> +               .parent_names = (const char *[]){ "xtal" },
> +               .num_parents = 1,
> +               .flags = CLK_IGNORE_UNUSED,
> +       },
> +};
> +
>  static unsigned int gxbb_aoclk_reset[] = {
>         [RESET_AO_REMOTE] = 16,
>         [RESET_AO_I2C_MASTER] = 18,
> @@ -131,8 +143,9 @@ static int gxbb_aoclk_do_reset(struct reset_controller_dev *rcdev,
>                 [CLKID_AO_UART1] = &uart1_ao.hw,
>                 [CLKID_AO_UART2] = &uart2_ao.hw,
>                 [CLKID_AO_IR_BLASTER] = &ir_blaster_ao.hw,
> +               [CLKID_AO_CEC_32K] = &cec_32k_ao.hw,
>         },
> -       .num = ARRAY_SIZE(gxbb_aoclk_gate),
> +       .num = 7,
>  };
>
>  static int gxbb_aoclkc_probe(struct platform_device *pdev)
> @@ -172,6 +185,12 @@ static int gxbb_aoclkc_probe(struct platform_device *pdev)
>                         return ret;
>         }
>
> +       /* Specific clocks */
> +       cec_32k_ao.regmap = regmap;
> +       ret = devm_clk_hw_register(dev, &cec_32k_ao.hw);
> +       if (ret)
> +               return ret;
> +
>         return of_clk_add_hw_provider(dev->of_node, of_clk_hw_onecell_get,
>                         &gxbb_aoclk_onecell_data);
>  }
> diff --git a/drivers/clk/meson/gxbb-aoclk.h b/drivers/clk/meson/gxbb-aoclk.h
> index 2e26108..e8604c8 100644
> --- a/drivers/clk/meson/gxbb-aoclk.h
> +++ b/drivers/clk/meson/gxbb-aoclk.h
> @@ -9,7 +9,13 @@
>  #define __GXBB_AOCLKC_H
>
>  /* AO Configuration Clock registers offsets */
> +#define AO_RTI_PWR_CNTL_REG1   0x0c
> +#define AO_RTI_PWR_CNTL_REG0   0x10
>  #define AO_RTI_GEN_CNTL_REG0   0x40
> +#define AO_OSCIN_CNTL          0x58
> +#define AO_CRT_CLK_CNTL1       0x68
> +#define AO_RTC_ALT_CLK_CNTL0   0x94
> +#define AO_RTC_ALT_CLK_CNTL1   0x98
>
>  struct aoclk_gate_regmap {
>         struct clk_hw hw;
> @@ -23,4 +29,14 @@ struct aoclk_gate_regmap {
>
>  extern const struct clk_ops meson_aoclk_gate_regmap_ops;
>
> +struct aoclk_cec_32k {
> +       struct clk_hw hw;
> +       struct regmap *regmap;
> +       spinlock_t *lock;
> +};
> +
> +#define to_aoclk_cec_32k(_hw) container_of(_hw, struct aoclk_cec_32k, hw)
> +
> +extern const struct clk_ops meson_aoclk_cec_32k_ops;
> +
>  #endif /* __GXBB_AOCLKC_H */
> --
> 1.9.1
>
>
> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v2 4/4] clk: meson: gxbb-aoclk: Add CEC 32k clock

[Neil Armstrong]

On 07/28/2017 12:29 PM, Martin Blumenstingl wrote:
> Hi Neil,
> 
> thanks for these patches, CEC support is another good step!

Hi Martin,

Thanks for your feedback !

> 
> On Fri, Jul 28, 2017 at 11:53 AM, Neil Armstrong
> <narmstrong@baylibre.com> wrote:
>> The CEC 32K AO Clock is a dual divider with dual counter to provide a more
>> precise 32768Hz clock for the CEC subsystem from the external xtal.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/clk/meson/Makefile         |   2 +-
>>  drivers/clk/meson/gxbb-aoclk-32k.c | 180 +++++++++++++++++++++++++++++++++++++
>>  drivers/clk/meson/gxbb-aoclk.c     |  21 ++++-
>>  drivers/clk/meson/gxbb-aoclk.h     |  16 ++++
>>  4 files changed, 217 insertions(+), 2 deletions(-)
>>  create mode 100644 drivers/clk/meson/gxbb-aoclk-32k.c
>>
>> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
>> index de65427..b139d41 100644
>> --- a/drivers/clk/meson/Makefile
>> +++ b/drivers/clk/meson/Makefile
>> @@ -4,4 +4,4 @@
>>
>>  obj-$(CONFIG_COMMON_CLK_AMLOGIC) += clk-pll.o clk-cpu.o clk-mpll.o clk-audio-divider.o
>>  obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o
>> -obj-$(CONFIG_COMMON_CLK_GXBB)   += gxbb.o gxbb-aoclk.o gxbb-aoclk-regmap.o
>> +obj-$(CONFIG_COMMON_CLK_GXBB)   += gxbb.o gxbb-aoclk.o gxbb-aoclk-regmap.o gxbb-aoclk-32k.o
>> diff --git a/drivers/clk/meson/gxbb-aoclk-32k.c b/drivers/clk/meson/gxbb-aoclk-32k.c
>> new file mode 100644
>> index 0000000..497cd09
>> --- /dev/null
>> +++ b/drivers/clk/meson/gxbb-aoclk-32k.c
>> @@ -0,0 +1,180 @@
>> +/*
>> + * Copyright (c) 2017 BayLibre, SAS.
>> + * Author: Neil Armstrong <narmstrong@baylibre.com>
>> + *
>> + * SPDX-License-Identifier: GPL-2.0+
>> + */
>> +
>> +#include <linux/clk-provider.h>
>> +#include <linux/bitfield.h>
>> +#include <linux/regmap.h>
>> +#include "gxbb-aoclk.h"
>> +
>> +/*
>> + * The AO Domain embeds a dual/divider to generate a more precise
>> + * 32,768KHz clock for low-power suspend mode and CEC.
>> + */
>> +
>> +#define CLK_CNTL0_N1_MASK      GENMASK(11, 0)
>> +#define CLK_CNTL0_N2_MASK      GENMASK(23, 12)
>> +#define CLK_CNTL0_DUALDIV_EN   BIT(28)
>> +#define CLK_CNTL0_OUT_GATE_EN  BIT(30)
>> +#define CLK_CNTL0_IN_GATE_EN   BIT(31)
>> +
>> +#define CLK_CNTL1_M1_MASK      GENMASK(11, 0)
>> +#define CLK_CNTL1_M2_MASK      GENMASK(23, 12)
>> +#define CLK_CNTL1_BYPASS_EN    BIT(24)
>> +#define CLK_CNTL1_SELECT_OSC   BIT(27)
>> +
>> +#define PWR_CNTL_ALT_32K_SEL   GENMASK(13, 10)
>> +
>> +struct cec_32k_freq_table {
>> +       unsigned long parent_rate;
>> +       unsigned long target_rate;
>> +       bool dualdiv;
>> +       unsigned int n1;
>> +       unsigned int n2;
>> +       unsigned int m1;
>> +       unsigned int m2;
>> +};
>> +
>> +static const struct cec_32k_freq_table aoclk_cec_32k_table[] = {
>> +       [0] = {
>> +               .parent_rate = 24000000,
> shouldn't you add a parent (XTAL) to this new clock so you could get
> rid of this?

It is in the clock definition in gxbb-aoclk.c.
This table only defines the "valid" dividing values given by amlogic.
Having 24MHz enforces we uses a 24MHz xtal as parent...
Once we figure out/have a valid documentation, we could fill it with
generic value and have a generic calculation.

> 
>> +               .target_rate = 32768,
>> +               .dualdiv = true,
>> +               .n1 = 733,
>> +               .n2 = 732,
>> +               .m1 = 8,
>> +               .m2 = 11,
>> +       },
>> +};
>> +
>> +/*
>> + * If CLK_CNTL0_DUALDIV_EN == 0
>> + *  - will use N1 divider only
>> + * If CLK_CNTL0_DUALDIV_EN == 1
>> + *  - hold M1 cycles of N1 divider then changes to N2
>> + *  - hold M2 cycles of N2 divider then changes to N1
>> + * Then we can get more accurate division.
>> + */
>> +static unsigned long aoclk_cec_32k_recalc_rate(struct clk_hw *hw,
>> +                                              unsigned long parent_rate)
>> +{
>> +       struct aoclk_cec_32k *cec_32k = to_aoclk_cec_32k(hw);
>> +       unsigned long n1;
>> +       u32 reg0, reg1;
>> +
>> +       regmap_read(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL0, &reg0);
>> +       regmap_read(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL1, &reg1);
>> +
>> +       if (reg1 & CLK_CNTL1_BYPASS_EN)
>> +               return parent_rate;
>> +
>> +       if (reg0 & CLK_CNTL0_DUALDIV_EN) {
>> +               unsigned long n2, m1, m2, f1, f2, p1, p2;
>> +
>> +               n1 = FIELD_GET(CLK_CNTL0_N1_MASK, reg0) + 1;
>> +               n2 = FIELD_GET(CLK_CNTL0_N2_MASK, reg0) + 1;
>> +
>> +               m1 = FIELD_GET(CLK_CNTL1_M1_MASK, reg1) + 1;
>> +               m2 = FIELD_GET(CLK_CNTL1_M2_MASK, reg1) + 1;
>> +
>> +               f1 = DIV_ROUND_CLOSEST(parent_rate, n1);
>> +               f2 = DIV_ROUND_CLOSEST(parent_rate, n2);
>> +
>> +               p1 = DIV_ROUND_CLOSEST(100000000 * m1, f1 * (m1 + m2));
>> +               p2 = DIV_ROUND_CLOSEST(100000000 * m2, f2 * (m1 + m2));
>> +
>> +               return DIV_ROUND_UP(100000000, p1 + p2);
>> +       }
>> +
>> +       n1 = FIELD_GET(CLK_CNTL0_N1_MASK, reg0) + 1;
>> +
>> +       return DIV_ROUND_CLOSEST(parent_rate, n1);
>> +}
>> +
>> +static const struct cec_32k_freq_table *find_cec_32k_freq(unsigned long rate,
>> +                                                         unsigned long prate)
>> +{
>> +       int i;
>> +
>> +       for (i = 0 ; i < ARRAY_SIZE(aoclk_cec_32k_table) ; ++i)
>> +               if (aoclk_cec_32k_table[i].parent_rate == prate &&
>> +                   aoclk_cec_32k_table[i].target_rate == rate)
>> +                       return &aoclk_cec_32k_table[i];
>> +
>> +       return NULL;
>> +}
>> +
>> +static long aoclk_cec_32k_round_rate(struct clk_hw *hw, unsigned long rate,
>> +                                    unsigned long *prate)
>> +{
>> +       const struct cec_32k_freq_table *freq = find_cec_32k_freq(rate,
>> +                                                                 *prate);
>> +
>> +       /* If invalid return first one */
>> +       if (!freq)
>> +               return freq[0].target_rate;
>> +
>> +       return freq->target_rate;
>> +}
>> +
>> +static int aoclk_cec_32k_set_rate(struct clk_hw *hw, unsigned long rate,
>> +                                 unsigned long parent_rate)
>> +{
>> +       const struct cec_32k_freq_table *freq = find_cec_32k_freq(rate,
>> +                                                                 parent_rate);
>> +       struct aoclk_cec_32k *cec_32k = to_aoclk_cec_32k(hw);
>> +       u32 reg = 0;
>> +
>> +       if (!freq)
>> +               return -EINVAL;
>> +
>> +       /* Disable clock */
>> +       regmap_update_bits(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL0,
>> +                          CLK_CNTL0_IN_GATE_EN | CLK_CNTL0_OUT_GATE_EN, 0);
>> +
>> +       if (freq->dualdiv)
>> +               reg = CLK_CNTL0_DUALDIV_EN |
>> +                     FIELD_PREP(CLK_CNTL0_N1_MASK, freq->n1 - 1) |
>> +                     FIELD_PREP(CLK_CNTL0_N2_MASK, freq->n2 - 1);
>> +       else
>> +               reg = FIELD_PREP(CLK_CNTL0_N1_MASK, freq->n1 - 1);
>> +
>> +       regmap_write(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL0, reg);
>> +
>> +       if (freq->dualdiv)
>> +               reg = FIELD_PREP(CLK_CNTL1_M1_MASK, freq->m1 - 1) |
>> +                     FIELD_PREP(CLK_CNTL1_M2_MASK, freq->m2 - 1);
>> +       else
>> +               reg = FIELD_PREP(CLK_CNTL1_M1_MASK, freq->m1 - 1);
>> +
>> +       regmap_write(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL1, reg);
>> +
>> +       /* Enable clock */
>> +       regmap_update_bits(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL0,
>> +                          CLK_CNTL0_IN_GATE_EN, CLK_CNTL0_IN_GATE_EN);
> based on how I understand your code the *input* clock needs to be
> gated during rate change, so (in my opinion) it's fine to toggle it
> here.
> 
>> +
>> +       udelay(200);
>> +
>> +       regmap_update_bits(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL0,
>> +                          CLK_CNTL0_OUT_GATE_EN, CLK_CNTL0_OUT_GATE_EN);
> however, should we make the output clock configurable by implementing
> clk_ops.enable and clk_ops.disable?

Yes, but in a perfect world we should need 2 gates, one before and one after
but it's a pain to synchronize and it's useless to have such complexity for
s clock that will only ever be used for CEC...

> 
>> +
>> +       regmap_update_bits(cec_32k->regmap, AO_CRT_CLK_CNTL1,
>> +                          CLK_CNTL1_SELECT_OSC, CLK_CNTL1_SELECT_OSC);
>> +
>> +       /* Select 32k from XTAL */
>> +       regmap_update_bits(cec_32k->regmap,
>> +                         AO_RTI_PWR_CNTL_REG0,
>> +                         PWR_CNTL_ALT_32K_SEL,
>> +                         FIELD_PREP(PWR_CNTL_ALT_32K_SEL, 4));
> is it correct to configure the muxes after enabling the clock output?
> also do you know about the other parents of PWR_CNTL_ALT_32K_SEL?

This mux is from some alternative GPIO input lines, but this exact clock
configuration is used by the suspend firmware...

Since there is a lot of unknowns about this clock, I copied the exact
sequence from amlogic code, but yes it should be modelized with a mux
and a dual gate somehow. But the firmware relies on it for low-freq
suspend mode and we won't have any other usage apart CEC under linux.

IMHO I think it's safer to keep this sequence.

(Anyway, it's not too late to change the clock model if we keep the
same DT bindings)

> 
>> +
>> +       return 0;
>> +}
>> +
>> +const struct clk_ops meson_aoclk_cec_32k_ops = {
>> +       .recalc_rate = aoclk_cec_32k_recalc_rate,
>> +       .round_rate = aoclk_cec_32k_round_rate,
>> +       .set_rate = aoclk_cec_32k_set_rate,
>> +};
>> diff --git a/drivers/clk/meson/gxbb-aoclk.c b/drivers/clk/meson/gxbb-aoclk.c
>> index f61506c..6c161e0 100644
>> --- a/drivers/clk/meson/gxbb-aoclk.c
>> +++ b/drivers/clk/meson/gxbb-aoclk.c
>> @@ -59,6 +59,7 @@
>>  #include <linux/mfd/syscon.h>
>>  #include <linux/regmap.h>
>>  #include <linux/init.h>
>> +#include <linux/delay.h>
>>  #include <dt-bindings/clock/gxbb-aoclkc.h>
>>  #include <dt-bindings/reset/gxbb-aoclkc.h>
>>  #include "gxbb-aoclk.h"
>> @@ -105,6 +106,17 @@ static int gxbb_aoclk_do_reset(struct reset_controller_dev *rcdev,
>>  GXBB_AO_GATE(uart2, 5);
>>  GXBB_AO_GATE(ir_blaster, 6);
>>
>> +static struct aoclk_cec_32k cec_32k_ao = {
>> +       .lock = &gxbb_aoclk_lock,
>> +       .hw.init = &(struct clk_init_data) {
>> +               .name = "cec_32k_ao",
>> +               .ops = &meson_aoclk_cec_32k_ops,
>> +               .parent_names = (const char *[]){ "xtal" },
>> +               .num_parents = 1,
>> +               .flags = CLK_IGNORE_UNUSED,
>> +       },
>> +};
>> +
>>  static unsigned int gxbb_aoclk_reset[] = {
>>         [RESET_AO_REMOTE] = 16,
>>         [RESET_AO_I2C_MASTER] = 18,
>> @@ -131,8 +143,9 @@ static int gxbb_aoclk_do_reset(struct reset_controller_dev *rcdev,
>>                 [CLKID_AO_UART1] = &uart1_ao.hw,
>>                 [CLKID_AO_UART2] = &uart2_ao.hw,
>>                 [CLKID_AO_IR_BLASTER] = &ir_blaster_ao.hw,
>> +               [CLKID_AO_CEC_32K] = &cec_32k_ao.hw,
>>         },
>> -       .num = ARRAY_SIZE(gxbb_aoclk_gate),
>> +       .num = 7,
>>  };
>>
>>  static int gxbb_aoclkc_probe(struct platform_device *pdev)
>> @@ -172,6 +185,12 @@ static int gxbb_aoclkc_probe(struct platform_device *pdev)
>>                         return ret;
>>         }
>>
>> +       /* Specific clocks */
>> +       cec_32k_ao.regmap = regmap;
>> +       ret = devm_clk_hw_register(dev, &cec_32k_ao.hw);
>> +       if (ret)
>> +               return ret;
>> +
>>         return of_clk_add_hw_provider(dev->of_node, of_clk_hw_onecell_get,
>>                         &gxbb_aoclk_onecell_data);
>>  }
>> diff --git a/drivers/clk/meson/gxbb-aoclk.h b/drivers/clk/meson/gxbb-aoclk.h
>> index 2e26108..e8604c8 100644
>> --- a/drivers/clk/meson/gxbb-aoclk.h
>> +++ b/drivers/clk/meson/gxbb-aoclk.h
>> @@ -9,7 +9,13 @@
>>  #define __GXBB_AOCLKC_H
>>
>>  /* AO Configuration Clock registers offsets */
>> +#define AO_RTI_PWR_CNTL_REG1   0x0c
>> +#define AO_RTI_PWR_CNTL_REG0   0x10
>>  #define AO_RTI_GEN_CNTL_REG0   0x40
>> +#define AO_OSCIN_CNTL          0x58
>> +#define AO_CRT_CLK_CNTL1       0x68
>> +#define AO_RTC_ALT_CLK_CNTL0   0x94
>> +#define AO_RTC_ALT_CLK_CNTL1   0x98
>>
>>  struct aoclk_gate_regmap {
>>         struct clk_hw hw;
>> @@ -23,4 +29,14 @@ struct aoclk_gate_regmap {
>>
>>  extern const struct clk_ops meson_aoclk_gate_regmap_ops;
>>
>> +struct aoclk_cec_32k {
>> +       struct clk_hw hw;
>> +       struct regmap *regmap;
>> +       spinlock_t *lock;
>> +};
>> +
>> +#define to_aoclk_cec_32k(_hw) container_of(_hw, struct aoclk_cec_32k, hw)
>> +
>> +extern const struct clk_ops meson_aoclk_cec_32k_ops;
>> +
>>  #endif /* __GXBB_AOCLKC_H */
>> --
>> 1.9.1
>>
>>
>> _______________________________________________
>> linux-amlogic mailing list
>> linux-amlogic@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v2 4/4] clk: meson: gxbb-aoclk: Add CEC 32k clock

[Martin Blumenstingl]

Hi Neil,

On Fri, Jul 28, 2017 at 4:09 PM, Neil Armstrong <narmstrong@baylibre.com> wrote:
> On 07/28/2017 12:29 PM, Martin Blumenstingl wrote:
>> Hi Neil,
>>
>> thanks for these patches, CEC support is another good step!
>
> Hi Martin,
>
> Thanks for your feedback !
>
>>
>> On Fri, Jul 28, 2017 at 11:53 AM, Neil Armstrong
>> <narmstrong@baylibre.com> wrote:
>>> The CEC 32K AO Clock is a dual divider with dual counter to provide a more
>>> precise 32768Hz clock for the CEC subsystem from the external xtal.
>>>
>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>> ---
>>>  drivers/clk/meson/Makefile         |   2 +-
>>>  drivers/clk/meson/gxbb-aoclk-32k.c | 180 +++++++++++++++++++++++++++++++++++++
>>>  drivers/clk/meson/gxbb-aoclk.c     |  21 ++++-
>>>  drivers/clk/meson/gxbb-aoclk.h     |  16 ++++
>>>  4 files changed, 217 insertions(+), 2 deletions(-)
>>>  create mode 100644 drivers/clk/meson/gxbb-aoclk-32k.c
>>>
>>> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
>>> index de65427..b139d41 100644
>>> --- a/drivers/clk/meson/Makefile
>>> +++ b/drivers/clk/meson/Makefile
>>> @@ -4,4 +4,4 @@
>>>
>>>  obj-$(CONFIG_COMMON_CLK_AMLOGIC) += clk-pll.o clk-cpu.o clk-mpll.o clk-audio-divider.o
>>>  obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o
>>> -obj-$(CONFIG_COMMON_CLK_GXBB)   += gxbb.o gxbb-aoclk.o gxbb-aoclk-regmap.o
>>> +obj-$(CONFIG_COMMON_CLK_GXBB)   += gxbb.o gxbb-aoclk.o gxbb-aoclk-regmap.o gxbb-aoclk-32k.o
>>> diff --git a/drivers/clk/meson/gxbb-aoclk-32k.c b/drivers/clk/meson/gxbb-aoclk-32k.c
>>> new file mode 100644
>>> index 0000000..497cd09
>>> --- /dev/null
>>> +++ b/drivers/clk/meson/gxbb-aoclk-32k.c
>>> @@ -0,0 +1,180 @@
>>> +/*
>>> + * Copyright (c) 2017 BayLibre, SAS.
>>> + * Author: Neil Armstrong <narmstrong@baylibre.com>
>>> + *
>>> + * SPDX-License-Identifier: GPL-2.0+
>>> + */
>>> +
>>> +#include <linux/clk-provider.h>
>>> +#include <linux/bitfield.h>
>>> +#include <linux/regmap.h>
>>> +#include "gxbb-aoclk.h"
>>> +
>>> +/*
>>> + * The AO Domain embeds a dual/divider to generate a more precise
>>> + * 32,768KHz clock for low-power suspend mode and CEC.
>>> + */
>>> +
>>> +#define CLK_CNTL0_N1_MASK      GENMASK(11, 0)
>>> +#define CLK_CNTL0_N2_MASK      GENMASK(23, 12)
>>> +#define CLK_CNTL0_DUALDIV_EN   BIT(28)
>>> +#define CLK_CNTL0_OUT_GATE_EN  BIT(30)
>>> +#define CLK_CNTL0_IN_GATE_EN   BIT(31)
>>> +
>>> +#define CLK_CNTL1_M1_MASK      GENMASK(11, 0)
>>> +#define CLK_CNTL1_M2_MASK      GENMASK(23, 12)
>>> +#define CLK_CNTL1_BYPASS_EN    BIT(24)
>>> +#define CLK_CNTL1_SELECT_OSC   BIT(27)
>>> +
>>> +#define PWR_CNTL_ALT_32K_SEL   GENMASK(13, 10)
>>> +
>>> +struct cec_32k_freq_table {
>>> +       unsigned long parent_rate;
>>> +       unsigned long target_rate;
>>> +       bool dualdiv;
>>> +       unsigned int n1;
>>> +       unsigned int n2;
>>> +       unsigned int m1;
>>> +       unsigned int m2;
>>> +};
>>> +
>>> +static const struct cec_32k_freq_table aoclk_cec_32k_table[] = {
>>> +       [0] = {
>>> +               .parent_rate = 24000000,
>> shouldn't you add a parent (XTAL) to this new clock so you could get
>> rid of this?
>
> It is in the clock definition in gxbb-aoclk.c.
> This table only defines the "valid" dividing values given by amlogic.
> Having 24MHz enforces we uses a 24MHz xtal as parent...
> Once we figure out/have a valid documentation, we could fill it with
> generic value and have a generic calculation.
ah, I missed that. in this case I guess it's fine

>>
>>> +               .target_rate = 32768,
>>> +               .dualdiv = true,
>>> +               .n1 = 733,
>>> +               .n2 = 732,
>>> +               .m1 = 8,
>>> +               .m2 = 11,
>>> +       },
>>> +};
>>> +
>>> +/*
>>> + * If CLK_CNTL0_DUALDIV_EN == 0
>>> + *  - will use N1 divider only
>>> + * If CLK_CNTL0_DUALDIV_EN == 1
>>> + *  - hold M1 cycles of N1 divider then changes to N2
>>> + *  - hold M2 cycles of N2 divider then changes to N1
>>> + * Then we can get more accurate division.
>>> + */
>>> +static unsigned long aoclk_cec_32k_recalc_rate(struct clk_hw *hw,
>>> +                                              unsigned long parent_rate)
>>> +{
>>> +       struct aoclk_cec_32k *cec_32k = to_aoclk_cec_32k(hw);
>>> +       unsigned long n1;
>>> +       u32 reg0, reg1;
>>> +
>>> +       regmap_read(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL0, &reg0);
>>> +       regmap_read(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL1, &reg1);
>>> +
>>> +       if (reg1 & CLK_CNTL1_BYPASS_EN)
>>> +               return parent_rate;
>>> +
>>> +       if (reg0 & CLK_CNTL0_DUALDIV_EN) {
>>> +               unsigned long n2, m1, m2, f1, f2, p1, p2;
>>> +
>>> +               n1 = FIELD_GET(CLK_CNTL0_N1_MASK, reg0) + 1;
>>> +               n2 = FIELD_GET(CLK_CNTL0_N2_MASK, reg0) + 1;
>>> +
>>> +               m1 = FIELD_GET(CLK_CNTL1_M1_MASK, reg1) + 1;
>>> +               m2 = FIELD_GET(CLK_CNTL1_M2_MASK, reg1) + 1;
>>> +
>>> +               f1 = DIV_ROUND_CLOSEST(parent_rate, n1);
>>> +               f2 = DIV_ROUND_CLOSEST(parent_rate, n2);
>>> +
>>> +               p1 = DIV_ROUND_CLOSEST(100000000 * m1, f1 * (m1 + m2));
>>> +               p2 = DIV_ROUND_CLOSEST(100000000 * m2, f2 * (m1 + m2));
>>> +
>>> +               return DIV_ROUND_UP(100000000, p1 + p2);
>>> +       }
>>> +
>>> +       n1 = FIELD_GET(CLK_CNTL0_N1_MASK, reg0) + 1;
>>> +
>>> +       return DIV_ROUND_CLOSEST(parent_rate, n1);
>>> +}
>>> +
>>> +static const struct cec_32k_freq_table *find_cec_32k_freq(unsigned long rate,
>>> +                                                         unsigned long prate)
>>> +{
>>> +       int i;
>>> +
>>> +       for (i = 0 ; i < ARRAY_SIZE(aoclk_cec_32k_table) ; ++i)
>>> +               if (aoclk_cec_32k_table[i].parent_rate == prate &&
>>> +                   aoclk_cec_32k_table[i].target_rate == rate)
>>> +                       return &aoclk_cec_32k_table[i];
>>> +
>>> +       return NULL;
>>> +}
>>> +
>>> +static long aoclk_cec_32k_round_rate(struct clk_hw *hw, unsigned long rate,
>>> +                                    unsigned long *prate)
>>> +{
>>> +       const struct cec_32k_freq_table *freq = find_cec_32k_freq(rate,
>>> +                                                                 *prate);
>>> +
>>> +       /* If invalid return first one */
>>> +       if (!freq)
>>> +               return freq[0].target_rate;
>>> +
>>> +       return freq->target_rate;
>>> +}
>>> +
>>> +static int aoclk_cec_32k_set_rate(struct clk_hw *hw, unsigned long rate,
>>> +                                 unsigned long parent_rate)
>>> +{
>>> +       const struct cec_32k_freq_table *freq = find_cec_32k_freq(rate,
>>> +                                                                 parent_rate);
>>> +       struct aoclk_cec_32k *cec_32k = to_aoclk_cec_32k(hw);
>>> +       u32 reg = 0;
>>> +
>>> +       if (!freq)
>>> +               return -EINVAL;
>>> +
>>> +       /* Disable clock */
>>> +       regmap_update_bits(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL0,
>>> +                          CLK_CNTL0_IN_GATE_EN | CLK_CNTL0_OUT_GATE_EN, 0);
>>> +
>>> +       if (freq->dualdiv)
>>> +               reg = CLK_CNTL0_DUALDIV_EN |
>>> +                     FIELD_PREP(CLK_CNTL0_N1_MASK, freq->n1 - 1) |
>>> +                     FIELD_PREP(CLK_CNTL0_N2_MASK, freq->n2 - 1);
>>> +       else
>>> +               reg = FIELD_PREP(CLK_CNTL0_N1_MASK, freq->n1 - 1);
>>> +
>>> +       regmap_write(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL0, reg);
>>> +
>>> +       if (freq->dualdiv)
>>> +               reg = FIELD_PREP(CLK_CNTL1_M1_MASK, freq->m1 - 1) |
>>> +                     FIELD_PREP(CLK_CNTL1_M2_MASK, freq->m2 - 1);
>>> +       else
>>> +               reg = FIELD_PREP(CLK_CNTL1_M1_MASK, freq->m1 - 1);
>>> +
>>> +       regmap_write(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL1, reg);
>>> +
>>> +       /* Enable clock */
>>> +       regmap_update_bits(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL0,
>>> +                          CLK_CNTL0_IN_GATE_EN, CLK_CNTL0_IN_GATE_EN);
>> based on how I understand your code the *input* clock needs to be
>> gated during rate change, so (in my opinion) it's fine to toggle it
>> here.
>>
>>> +
>>> +       udelay(200);
>>> +
>>> +       regmap_update_bits(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL0,
>>> +                          CLK_CNTL0_OUT_GATE_EN, CLK_CNTL0_OUT_GATE_EN);
>> however, should we make the output clock configurable by implementing
>> clk_ops.enable and clk_ops.disable?
>
> Yes, but in a perfect world we should need 2 gates, one before and one after
> but it's a pain to synchronize and it's useless to have such complexity for
> s clock that will only ever be used for CEC...
the reason why I asked is the case where someone doesn't want to use
HDMI (some people on IRC were asking how to reclaim the memory which
is reserved for the framebuffer). I'm not sure if it would be good to
keep at least the output disabled for that case.
on the other hand one has to be careful then: I don't know if the
suspend firmware re-enables this clock (else we need to make sure that
we don't disable this clock during suspend).
I'll leave it up to you whether you want to change it or not (if we
really need it we can still implement these callbacks later).

>>
>>> +
>>> +       regmap_update_bits(cec_32k->regmap, AO_CRT_CLK_CNTL1,
>>> +                          CLK_CNTL1_SELECT_OSC, CLK_CNTL1_SELECT_OSC);
>>> +
>>> +       /* Select 32k from XTAL */
>>> +       regmap_update_bits(cec_32k->regmap,
>>> +                         AO_RTI_PWR_CNTL_REG0,
>>> +                         PWR_CNTL_ALT_32K_SEL,
>>> +                         FIELD_PREP(PWR_CNTL_ALT_32K_SEL, 4));
>> is it correct to configure the muxes after enabling the clock output?
>> also do you know about the other parents of PWR_CNTL_ALT_32K_SEL?
>
> This mux is from some alternative GPIO input lines, but this exact clock
> configuration is used by the suspend firmware...
>
> Since there is a lot of unknowns about this clock, I copied the exact
> sequence from amlogic code, but yes it should be modelized with a mux
> and a dual gate somehow. But the firmware relies on it for low-freq
> suspend mode and we won't have any other usage apart CEC under linux.
>
> IMHO I think it's safer to keep this sequence.
if you have to re-spin this due to whatever reason: could you please
add a comment with that information (that this matches what the
suspend firmware does and that there's currently no documentation
available -> that should prevent people from messing with these
registers)

> (Anyway, it's not too late to change the clock model if we keep the
> same DT bindings)
indeed, that is basically the reason why I'm asking: if we have to
consider something now to keep the DT bindings stable then we should
look into it.

>>
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +const struct clk_ops meson_aoclk_cec_32k_ops = {
>>> +       .recalc_rate = aoclk_cec_32k_recalc_rate,
>>> +       .round_rate = aoclk_cec_32k_round_rate,
>>> +       .set_rate = aoclk_cec_32k_set_rate,
>>> +};
>>> diff --git a/drivers/clk/meson/gxbb-aoclk.c b/drivers/clk/meson/gxbb-aoclk.c
>>> index f61506c..6c161e0 100644
>>> --- a/drivers/clk/meson/gxbb-aoclk.c
>>> +++ b/drivers/clk/meson/gxbb-aoclk.c
>>> @@ -59,6 +59,7 @@
>>>  #include <linux/mfd/syscon.h>
>>>  #include <linux/regmap.h>
>>>  #include <linux/init.h>
>>> +#include <linux/delay.h>
>>>  #include <dt-bindings/clock/gxbb-aoclkc.h>
>>>  #include <dt-bindings/reset/gxbb-aoclkc.h>
>>>  #include "gxbb-aoclk.h"
>>> @@ -105,6 +106,17 @@ static int gxbb_aoclk_do_reset(struct reset_controller_dev *rcdev,
>>>  GXBB_AO_GATE(uart2, 5);
>>>  GXBB_AO_GATE(ir_blaster, 6);
>>>
>>> +static struct aoclk_cec_32k cec_32k_ao = {
>>> +       .lock = &gxbb_aoclk_lock,
>>> +       .hw.init = &(struct clk_init_data) {
>>> +               .name = "cec_32k_ao",
>>> +               .ops = &meson_aoclk_cec_32k_ops,
>>> +               .parent_names = (const char *[]){ "xtal" },
>>> +               .num_parents = 1,
>>> +               .flags = CLK_IGNORE_UNUSED,
>>> +       },
>>> +};
>>> +
>>>  static unsigned int gxbb_aoclk_reset[] = {
>>>         [RESET_AO_REMOTE] = 16,
>>>         [RESET_AO_I2C_MASTER] = 18,
>>> @@ -131,8 +143,9 @@ static int gxbb_aoclk_do_reset(struct reset_controller_dev *rcdev,
>>>                 [CLKID_AO_UART1] = &uart1_ao.hw,
>>>                 [CLKID_AO_UART2] = &uart2_ao.hw,
>>>                 [CLKID_AO_IR_BLASTER] = &ir_blaster_ao.hw,
>>> +               [CLKID_AO_CEC_32K] = &cec_32k_ao.hw,
>>>         },
>>> -       .num = ARRAY_SIZE(gxbb_aoclk_gate),
>>> +       .num = 7,
>>>  };
>>>
>>>  static int gxbb_aoclkc_probe(struct platform_device *pdev)
>>> @@ -172,6 +185,12 @@ static int gxbb_aoclkc_probe(struct platform_device *pdev)
>>>                         return ret;
>>>         }
>>>
>>> +       /* Specific clocks */
>>> +       cec_32k_ao.regmap = regmap;
>>> +       ret = devm_clk_hw_register(dev, &cec_32k_ao.hw);
>>> +       if (ret)
>>> +               return ret;
>>> +
>>>         return of_clk_add_hw_provider(dev->of_node, of_clk_hw_onecell_get,
>>>                         &gxbb_aoclk_onecell_data);
>>>  }
>>> diff --git a/drivers/clk/meson/gxbb-aoclk.h b/drivers/clk/meson/gxbb-aoclk.h
>>> index 2e26108..e8604c8 100644
>>> --- a/drivers/clk/meson/gxbb-aoclk.h
>>> +++ b/drivers/clk/meson/gxbb-aoclk.h
>>> @@ -9,7 +9,13 @@
>>>  #define __GXBB_AOCLKC_H
>>>
>>>  /* AO Configuration Clock registers offsets */
>>> +#define AO_RTI_PWR_CNTL_REG1   0x0c
>>> +#define AO_RTI_PWR_CNTL_REG0   0x10
>>>  #define AO_RTI_GEN_CNTL_REG0   0x40
>>> +#define AO_OSCIN_CNTL          0x58
>>> +#define AO_CRT_CLK_CNTL1       0x68
>>> +#define AO_RTC_ALT_CLK_CNTL0   0x94
>>> +#define AO_RTC_ALT_CLK_CNTL1   0x98
>>>
>>>  struct aoclk_gate_regmap {
>>>         struct clk_hw hw;
>>> @@ -23,4 +29,14 @@ struct aoclk_gate_regmap {
>>>
>>>  extern const struct clk_ops meson_aoclk_gate_regmap_ops;
>>>
>>> +struct aoclk_cec_32k {
>>> +       struct clk_hw hw;
>>> +       struct regmap *regmap;
>>> +       spinlock_t *lock;
>>> +};
>>> +
>>> +#define to_aoclk_cec_32k(_hw) container_of(_hw, struct aoclk_cec_32k, hw)
>>> +
>>> +extern const struct clk_ops meson_aoclk_cec_32k_ops;
>>> +
>>>  #endif /* __GXBB_AOCLKC_H */
>>> --
>>> 1.9.1
>>>
>>>
>>> _______________________________________________
>>> linux-amlogic mailing list
>>> linux-amlogic@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-amlogic
>

Regards,
Martin

Re: [PATCH v2 4/4] clk: meson: gxbb-aoclk: Add CEC 32k clock

[Chris Moore]

Hi,

Sorry I forgot to reply to all in my previous post :(
I hope this corrects things.

Le 28/07/2017 à 11:53, Neil Armstrong a écrit :

[snip]

> +static long aoclk_cec_32k_round_rate(struct clk_hw *hw, unsigned long 
> rate,
> +                     unsigned long *prate)
> +{
> +    const struct cec_32k_freq_table *freq = find_cec_32k_freq(rate,
> +                                  *prate);
> +
> +    /* If invalid return first one */
> +    if (!freq)
> +        return freq[0].target_rate;

Wouldn't this dereference a null pointer (or am I being stupid this 
morning)?

> +
> +    return freq->target_rate;
> +}
> +
> +static int aoclk_cec_32k_set_rate(struct clk_hw *hw, unsigned long rate,
> +                  unsigned long parent_rate)
> +{
> +    const struct cec_32k_freq_table *freq = find_cec_32k_freq(rate,
> +                                  parent_rate);
> +    struct aoclk_cec_32k *cec_32k = to_aoclk_cec_32k(hw);
> +    u32 reg = 0;
> +
> +    if (!freq)
> +        return -EINVAL;
> +
> +    /* Disable clock */
> +    regmap_update_bits(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL0,
> +               CLK_CNTL0_IN_GATE_EN | CLK_CNTL0_OUT_GATE_EN, 0);
> +
> +    if (freq->dualdiv)
> +        reg = CLK_CNTL0_DUALDIV_EN |
> +              FIELD_PREP(CLK_CNTL0_N1_MASK, freq->n1 - 1) |
> +              FIELD_PREP(CLK_CNTL0_N2_MASK, freq->n2 - 1);
> +    else
> +        reg = FIELD_PREP(CLK_CNTL0_N1_MASK, freq->n1 - 1);
> +

Suggestion:

+    reg = FIELD_PREP(CLK_CNTL0_N1_MASK, freq->n1 - 1);
+    if (freq->dualdiv)
+        reg |= CLK_CNTL0_DUALDIV_EN |
+               FIELD_PREP(CLK_CNTL0_N2_MASK, freq->n2 - 1);

is shorter but maybe generates the same code.

> + regmap_write(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL0, reg);
> +
> +    if (freq->dualdiv)
> +        reg = FIELD_PREP(CLK_CNTL1_M1_MASK, freq->m1 - 1) |
> +              FIELD_PREP(CLK_CNTL1_M2_MASK, freq->m2 - 1);
> +    else
> +        reg = FIELD_PREP(CLK_CNTL1_M1_MASK, freq->m1 - 1);
> +

Idem:

+    reg = FIELD_PREP(CLK_CNTL1_M1_MASK, freq->m1 - 1);
+    if (freq->dualdiv)
+        reg |= FIELD_PREP(CLK_CNTL1_M2_MASK, freq->m2 - 1);

Cheers,
Chris

Re: [PATCH v2 4/4] clk: meson: gxbb-aoclk: Add CEC 32k clock

[Neil Armstrong]

Hi,

On 07/29/2017 10:04 AM, Chris Moore wrote:
> Hi,
> 
> Sorry I forgot to reply to all in my previous post :(
> I hope this corrects things.
> 
> Le 28/07/2017 à 11:53, Neil Armstrong a écrit :
> 
> [snip]
> 
>> +static long aoclk_cec_32k_round_rate(struct clk_hw *hw, unsigned long rate,
>> +                     unsigned long *prate)
>> +{
>> +    const struct cec_32k_freq_table *freq = find_cec_32k_freq(rate,
>> +                                  *prate);
>> +
>> +    /* If invalid return first one */
>> +    if (!freq)
>> +        return freq[0].target_rate;
> 
> Wouldn't this dereference a null pointer (or am I being stupid this morning)?

No, it's me !!
Will fix it in v3...

> 
>> +
>> +    return freq->target_rate;
>> +}
>> +
>> +static int aoclk_cec_32k_set_rate(struct clk_hw *hw, unsigned long rate,
>> +                  unsigned long parent_rate)
>> +{
>> +    const struct cec_32k_freq_table *freq = find_cec_32k_freq(rate,
>> +                                  parent_rate);
>> +    struct aoclk_cec_32k *cec_32k = to_aoclk_cec_32k(hw);
>> +    u32 reg = 0;
>> +
>> +    if (!freq)
>> +        return -EINVAL;
>> +
>> +    /* Disable clock */
>> +    regmap_update_bits(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL0,
>> +               CLK_CNTL0_IN_GATE_EN | CLK_CNTL0_OUT_GATE_EN, 0);
>> +
>> +    if (freq->dualdiv)
>> +        reg = CLK_CNTL0_DUALDIV_EN |
>> +              FIELD_PREP(CLK_CNTL0_N1_MASK, freq->n1 - 1) |
>> +              FIELD_PREP(CLK_CNTL0_N2_MASK, freq->n2 - 1);
>> +    else
>> +        reg = FIELD_PREP(CLK_CNTL0_N1_MASK, freq->n1 - 1);
>> +
> 
> Suggestion:
> 
> +    reg = FIELD_PREP(CLK_CNTL0_N1_MASK, freq->n1 - 1);
> +    if (freq->dualdiv)
> +        reg |= CLK_CNTL0_DUALDIV_EN |
> +               FIELD_PREP(CLK_CNTL0_N2_MASK, freq->n2 - 1);
> 
> is shorter but maybe generates the same code.
> 
>> + regmap_write(cec_32k->regmap, AO_RTC_ALT_CLK_CNTL0, reg);
>> +
>> +    if (freq->dualdiv)
>> +        reg = FIELD_PREP(CLK_CNTL1_M1_MASK, freq->m1 - 1) |
>> +              FIELD_PREP(CLK_CNTL1_M2_MASK, freq->m2 - 1);
>> +    else
>> +        reg = FIELD_PREP(CLK_CNTL1_M1_MASK, freq->m1 - 1);
>> +
> 
> Idem:
> 
> +    reg = FIELD_PREP(CLK_CNTL1_M1_MASK, freq->m1 - 1);
> +    if (freq->dualdiv)
> +        reg |= FIELD_PREP(CLK_CNTL1_M2_MASK, freq->m2 - 1);
>

Thanks for the suggestions, I will send a v3 asap.

Neil
> Cheers,
> Chris