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

[PATCH 0/3] 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
 - adds the CEC 32k clock
 - adds the clock binding entry

The DT Update will be sent in another patchset.

Neil Armstrong (3):
  dt-bindings: clock: gxbb-aoclk: Add CEC 32k clock
  clk: meson: gxbb-aoclk: Add CEC 32k clock
  dt-bindings: clock: amlogic,gxbb-aoclkc: Update bindings

 .../bindings/clock/amlogic,gxbb-aoclkc.txt         |  11 +-
 drivers/clk/meson/Makefile                         |   2 +-
 drivers/clk/meson/gxbb-aoclk-32k.c                 | 188 +++++++++++++++++++++
 drivers/clk/meson/gxbb-aoclk.c                     |  59 ++++++-
 drivers/clk/meson/gxbb-aoclk.h                     |  23 +++
 include/dt-bindings/clock/gxbb-aoclkc.h            |   1 +
 6 files changed, 275 insertions(+), 9 deletions(-)
 create mode 100644 drivers/clk/meson/gxbb-aoclk-32k.c
 create mode 100644 drivers/clk/meson/gxbb-aoclk.h

-- 
1.9.1

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

[Neil Armstrong]

This patch adds 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 2/3] 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.

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.

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

diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
index 83b6d9d..e315057 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-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..c37c76a
--- /dev/null
+++ b/drivers/clk/meson/gxbb-aoclk-32k.c
@@ -0,0 +1,188 @@
+/*
+ * 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 AO_RTC_ALT_CLK_CNTL0	0x0
+#define AO_RTC_ALT_CLK_CNTL1	0x4
+#define AO_CRT_CLK_CNTL1	0x0
+#define AO_RTI_PWR_CNTL_REG0	0x4
+
+#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;
+
+	reg0 = readl_relaxed(cec_32k->rtc_base + AO_RTC_ALT_CLK_CNTL0);
+	reg1 = readl_relaxed(cec_32k->rtc_base + AO_RTC_ALT_CLK_CNTL1);
+
+	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 */
+	reg = readl(cec_32k->rtc_base + AO_RTC_ALT_CLK_CNTL0);
+	reg &= ~(CLK_CNTL0_IN_GATE_EN | CLK_CNTL0_OUT_GATE_EN);
+	writel(reg, cec_32k->rtc_base + AO_RTC_ALT_CLK_CNTL0);
+
+	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);
+
+	writel_relaxed(reg, cec_32k->rtc_base + AO_RTC_ALT_CLK_CNTL0);
+
+	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);
+
+	writel_relaxed(reg, cec_32k->rtc_base + AO_RTC_ALT_CLK_CNTL1);
+
+	/* Enable clock */
+	reg = readl(cec_32k->rtc_base + AO_RTC_ALT_CLK_CNTL0);
+	reg |= CLK_CNTL0_IN_GATE_EN;
+	writel(reg, cec_32k->rtc_base + AO_RTC_ALT_CLK_CNTL0);
+
+	udelay(200);
+
+	reg |= CLK_CNTL0_OUT_GATE_EN;
+	writel(reg, cec_32k->rtc_base + AO_RTC_ALT_CLK_CNTL0);
+
+	reg = readl(cec_32k->crt_base + AO_CRT_CLK_CNTL1);
+	reg |= CLK_CNTL1_SELECT_OSC;	/* select cts_rtc_oscin_clk */
+	writel(reg, cec_32k->crt_base + AO_CRT_CLK_CNTL1);
+
+	/* Select 32k from XTAL */
+	regmap_write_bits(cec_32k->pwr_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 b45c5fb..e47f021 100644
--- a/drivers/clk/meson/gxbb-aoclk.c
+++ b/drivers/clk/meson/gxbb-aoclk.c
@@ -56,9 +56,13 @@
 #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 <linux/delay.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);
 
@@ -104,6 +108,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,
@@ -130,28 +145,52 @@ 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)
 {
-	struct resource *res;
+	struct gxbb_aoclk_reset_controller *rstc;
+	struct device *dev = &pdev->dev;
+	struct regmap *regmap_pwr;
+	void __iomem *base_crt;
+	void __iomem *base_rtc;
 	void __iomem *base;
+	struct resource *res;
 	int ret, clkid;
-	struct device *dev = &pdev->dev;
-	struct gxbb_aoclk_reset_controller *rstc;
 
 	rstc = devm_kzalloc(dev, sizeof(*rstc), GFP_KERNEL);
 	if (!rstc)
 		return -ENOMEM;
 
 	/* Generic clocks */
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "aoclk");
 	base = devm_ioremap_resource(dev, res);
 	if (IS_ERR(base))
 		return PTR_ERR(base);
 
+	/* CRT base */
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "aocrt");
+	base_crt = devm_ioremap_resource(dev, res);
+	if (IS_ERR(base_crt))
+		return PTR_ERR(base_crt);
+
+	/* RTC base */
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "aortc");
+	base_rtc = devm_ioremap_resource(dev, res);
+	if (IS_ERR(base_rtc))
+		return PTR_ERR(base_rtc);
+
+	/* PWR regmap */
+	regmap_pwr = syscon_regmap_lookup_by_phandle(dev->of_node,
+						     "amlogic,pwr-ctrl");
+	if (IS_ERR(regmap_pwr)) {
+		dev_err(dev, "failed to get PWR regmap\n");
+		return -ENODEV;
+	}
+
 	/* Reset Controller */
 	rstc->base = base;
 	rstc->data = gxbb_aoclk_reset;
@@ -163,7 +202,7 @@ static int gxbb_aoclkc_probe(struct platform_device *pdev)
 	/*
 	 * Populate base address and register all clks
 	 */
-	for (clkid = 0; clkid < gxbb_aoclk_onecell_data.num; clkid++) {
+	for (clkid = 0; clkid < ARRAY_SIZE(gxbb_aoclk_gate); clkid++) {
 		gxbb_aoclk_gate[clkid]->reg = base;
 
 		ret = devm_clk_hw_register(dev,
@@ -172,6 +211,14 @@ static int gxbb_aoclkc_probe(struct platform_device *pdev)
 			return ret;
 	}
 
+	/* Specific clocks */
+	cec_32k_ao.crt_base = base_crt;
+	cec_32k_ao.rtc_base = base_rtc;
+	cec_32k_ao.pwr_regmap = regmap_pwr;
+	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
new file mode 100644
index 0000000..5925a6b
--- /dev/null
+++ b/drivers/clk/meson/gxbb-aoclk.h
@@ -0,0 +1,23 @@
+/*
+ * 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
+
+struct aoclk_cec_32k {
+	struct clk_hw hw;
+	void __iomem *crt_base;
+	void __iomem *rtc_base;
+	struct regmap *pwr_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

[PATCH 3/3] 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 these registers to the Ao Clock bindings with direct access
and shared extcon access.

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

diff --git a/Documentation/devicetree/bindings/clock/amlogic,gxbb-aoclkc.txt b/Documentation/devicetree/bindings/clock/amlogic,gxbb-aoclkc.txt
index a55d31b..5c5ccec 100644
--- a/Documentation/devicetree/bindings/clock/amlogic,gxbb-aoclkc.txt
+++ b/Documentation/devicetree/bindings/clock/amlogic,gxbb-aoclkc.txt
@@ -7,7 +7,10 @@ Required Properties:
 
 - compatible: should be "amlogic,gxbb-aoclkc"
 - reg: physical base address of the clock controller and length of memory
-       mapped region.
+       mapped region for each registers listed in reg-names.
+- reg-names: should contain the following register names :
+	"aoclk", "aocrt" and "aortc".
+- amlogic,pwr-ctrl: A phandle to the AO Power Control node.
 
 - #clock-cells: should be 1.
 
@@ -27,9 +30,13 @@ Example: AO Clock controller node:
 
 	clkc_AO: clock-controller@040 {
 		compatible = "amlogic,gxbb-aoclkc";
-		reg = <0x0 0x040 0x0 0x4>;
+		reg = <0x0 0x00040 0x0 0x4>,
+		      <0x0 0x00068 0x0 0x4>,
+		      <0x0 0x00094 0x0 0x8>;
+		reg-names = "aoclk", "aocrt", "aortc";
 		#clock-cells = <1>;
 		#reset-cells = <1>;
+		amlogic,pwr-ctrl = <&pwr_AO>;
 	};
 
 Example: UART controller node that consumes the clock and reset generated
-- 
1.9.1

Re: [PATCH 3/3] dt-bindings: clock: amlogic,gxbb-aoclkc: Update bindings

[Rob Herring]

On Thu, Jul 06, 2017 at 12:24:23PM +0200, Neil Armstrong wrote:
> 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 these registers to the Ao Clock bindings with direct access
> and shared extcon access.
> 
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  .../devicetree/bindings/clock/amlogic,gxbb-aoclkc.txt         | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)

This looks like the binding might be too specific with a reg list of 
single registers, and you should define a system controller node 
instead. Depends on what else is in the "A0" block.

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 3/3] dt-bindings: clock: amlogic,gxbb-aoclkc: Update bindings

[Stephen Boyd]

On 07/09, Rob Herring wrote:
> On Thu, Jul 06, 2017 at 12:24:23PM +0200, Neil Armstrong wrote:
> > 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 these registers to the Ao Clock bindings with direct access
> > and shared extcon access.
> > 
> > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> > ---
> >  .../devicetree/bindings/clock/amlogic,gxbb-aoclkc.txt         | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> This looks like the binding might be too specific with a reg list of 
> single registers, and you should define a system controller node 
> instead. Depends on what else is in the "A0" block.
> 

Agreed. Why can't we expand the size in DT and then access the
registers directly in the driver. Hopefully it keeps working to
apply the dts patch without the driver patch too (and
vice-versa), because the kernel can only make a mapping as small
as a page which would cover these newly added reg properties
anyway.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 3/3] dt-bindings: clock: amlogic,gxbb-aoclkc: Update bindings

[Neil Armstrong]

On 07/21/2017 10:44 PM, Stephen Boyd wrote:
> On 07/09, Rob Herring wrote:
>> On Thu, Jul 06, 2017 at 12:24:23PM +0200, Neil Armstrong wrote:
>>> 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 these registers to the Ao Clock bindings with direct access
>>> and shared extcon access.
>>>
>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>> ---
>>>  .../devicetree/bindings/clock/amlogic,gxbb-aoclkc.txt         | 11 +++++++++--
>>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> This looks like the binding might be too specific with a reg list of 
>> single registers, and you should define a system controller node 
>> instead. Depends on what else is in the "A0" block.
>>
> 
> Agreed. Why can't we expand the size in DT and then access the
> registers directly in the driver. Hopefully it keeps working to
> apply the dts patch without the driver patch too (and
> vice-versa), because the kernel can only make a mapping as small
> as a page which would cover these newly added reg properties
> anyway.
> 

Hi Rob, Stephen,

Thanks for your feedback, but on these Amlogic platforms, the AO register bank is
filled with interleaved registers used for various purposes.

For instance, the first 1k of registers are :

0-c RTI_STATUS
c-14 RTI_PWR_CNTL
14-1c PIN_MUX
1c RTI_STATUS
24-30 GPIO
30 JTAG_CONFIG
34 WD
38-40 CPU_CTRL
40 RTI_GEN_CTRL
44 CPU_CTRL
4c-58 TIMER
58 OSCIN
60 AHB2DDR
...


And so on, and the clock related registers are split among this space.

For sure, this could be declared as an system controller node, but this would imply completely
re-designing the actual clock driver and drop the actual bindings.
(and with re-writting the clock gate code to use regmap registers access)

Anyway, I'm open to suggestions...

Neil

Re: [PATCH 3/3] dt-bindings: clock: amlogic,gxbb-aoclkc: Update bindings

[Jerome Brunet]

On Mon, 2017-07-24 at 13:47 +0200, Neil Armstrong wrote:
> On 07/21/2017 10:44 PM, Stephen Boyd wrote:
> > On 07/09, Rob Herring wrote:
> > > On Thu, Jul 06, 2017 at 12:24:23PM +0200, Neil Armstrong wrote:
> > > > 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 these registers to the Ao Clock bindings with direct
> > > > access
> > > > and shared extcon access.
> > > > 
> > > > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> > > > ---
> > > >  .../devicetree/bindings/clock/amlogic,gxbb-aoclkc.txt         | 11
> > > > +++++++++--
> > > >  1 file changed, 9 insertions(+), 2 deletions(-)
> > > 
> > > This looks like the binding might be too specific with a reg list of 
> > > single registers, and you should define a system controller node 
> > > instead. Depends on what else is in the "A0" block.
> > > 
> > 
> > Agreed. Why can't we expand the size in DT and then access the
> > registers directly in the driver. Hopefully it keeps working to
> > apply the dts patch without the driver patch too (and
> > vice-versa), because the kernel can only make a mapping as small
> > as a page which would cover these newly added reg properties
> > anyway.
> > 
> 
> Hi Rob, Stephen,
> 
> Thanks for your feedback, but on these Amlogic platforms, the AO register bank
> is
> filled with interleaved registers used for various purposes.
> 
> For instance, the first 1k of registers are :
> 
> 0-c RTI_STATUS
> c-14 RTI_PWR_CNTL
> 14-1c PIN_MUX
> 1c RTI_STATUS
> 24-30 GPIO
> 30 JTAG_CONFIG
> 34 WD
> 38-40 CPU_CTRL
> 40 RTI_GEN_CTRL
> 44 CPU_CTRL
> 4c-58 TIMER
> 58 OSCIN
> 60 AHB2DDR
> ...
> 
> 
> And so on, and the clock related registers are split among this space.
> 
> For sure, this could be declared as an system controller node, but this would
> imply completely
> re-designing the actual clock driver and drop the actual bindings.
> (and with re-writting the clock gate code to use regmap registers access)

Maybe it is time to investigate having the regmap clock from qcom available to
every other platform ?

> 
> Anyway, I'm open to suggestions...
> 
> Neil

Re: [PATCH 3/3] dt-bindings: clock: amlogic,gxbb-aoclkc: Update bindings

[Stephen Boyd]

On 07/24, Jerome Brunet wrote:
> On Mon, 2017-07-24 at 13:47 +0200, Neil Armstrong wrote:
> > 
> > Hi Rob, Stephen,
> > 
> > Thanks for your feedback, but on these Amlogic platforms, the AO register bank
> > is
> > filled with interleaved registers used for various purposes.
> > 
> > For instance, the first 1k of registers are :
> > 
> > 0-c RTI_STATUS
> > c-14 RTI_PWR_CNTL
> > 14-1c PIN_MUX
> > 1c RTI_STATUS
> > 24-30 GPIO
> > 30 JTAG_CONFIG
> > 34 WD
> > 38-40 CPU_CTRL
> > 40 RTI_GEN_CTRL
> > 44 CPU_CTRL
> > 4c-58 TIMER
> > 58 OSCIN
> > 60 AHB2DDR
> > ...
> > 
> > 
> > And so on, and the clock related registers are split among this space.
> > 
> > For sure, this could be declared as an system controller node, but this would
> > imply completely
> > re-designing the actual clock driver and drop the actual bindings.
> > (and with re-writting the clock gate code to use regmap registers access)

I'm not suggesting a sycon node or binding usage here. There's an
"always on" hw block here that could be implemented as an MFD
driver that binds and creates a clk subdevice and whatever else
is sitting in here. Those subdevices could be informed of the
register base by knowing their parent driver is an MFD with a
certain driver data structure inside and then get at an __iomem
pointer through the parent's driver data. Or they could use a
regmap approach and rewrite a bunch of code. Or there could be
just a clk driver that binds to this node for now until a later
time that other features are needed.

> 
> Maybe it is time to investigate having the regmap clock from qcom available to
> every other platform ?

I think we have regmap clk duplicated a couple times in the
drivers/clk/ directory now. Not sure how this is related, except
for that there looks to be a desire to use a syscon binding here
and that forces regmap on drivers?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 3/3] dt-bindings: clock: amlogic,gxbb-aoclkc: Update bindings

[Jerome Brunet]

On Tue, 2017-07-25 at 17:48 -0700, Stephen Boyd wrote:
> > 
> > Maybe it is time to investigate having the regmap clock from qcom available
> > to
> > every other platform ?
> 
> I think we have regmap clk duplicated a couple times in the
> drivers/clk/ directory now.

Which is why we may start thinking of common and generic "regmap" compatible
solution in the CCF, at least for things like gates, dividers and muxes

The approach used in qcom with regmap clocks could be a candidate for this,
don't you think ?

> Not sure how this is related, except
> for that there looks to be a desire to use a syscon binding here
> and that forces regmap on drivers?

Syscon has been created exactly for this case where you want to create an mfd
just for sharing a register region between several, otherwise well separated,
devices, isn't it ?