[PATCH] clk: bcm: Add driver for Northstar ILP clock

[PATCH] clk: bcm: Add driver for Northstar ILP clock

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

This clock is present on cheaper Northstar devices like BCM53573 or
BCM47189 using Corex-A7. This driver uses PMU (Power Management Unit)
to calculate clock rate and allows using it in a generic (clk_*) way.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 .../devicetree/bindings/clock/brcm,ns-ilp.txt      |  28 ++++
 drivers/clk/bcm/Makefile                           |   1 +
 drivers/clk/bcm/clk-ns-ilp.c                       | 146 +++++++++++++++++++++
 3 files changed, 175 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt
 create mode 100644 drivers/clk/bcm/clk-ns-ilp.c

diff --git a/Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt b/Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt
new file mode 100644
index 0000000..c4df38e
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt
@@ -0,0 +1,28 @@
+Broadcom Northstar ILP clock
+============================
+
+This binding uses the common clock binding:
+    Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+This binding is used for ILP clock on Broadcom Northstar devices using
+Corex-A7 CPU. ILP clock depends on ALP one and has to be calculated on
+runtime.
+
+Required properties:
+- compatible: "brcm,ns-ilp"
+- reg: iomem address range of PMU (Power Management Unit)
+- reg-names: "pmu", the only needed & supported reg right now
+- clocks: should reference an ALP clock
+- clock-names: "alp", the only needed & supported clock right now
+- #clock-cells: should be <0>
+
+Example:
+
+ilp: ilp {
+	compatible = "brcm,ns-ilp";
+	reg = <0x18012000 0x1000>;
+	reg-names = "pmu";
+	clocks = <&alp>;
+	clock-names = "alp-clk";
+	#clock-cells = <0>;
+};
diff --git a/drivers/clk/bcm/Makefile b/drivers/clk/bcm/Makefile
index 1d79bd2..1389379 100644
--- a/drivers/clk/bcm/Makefile
+++ b/drivers/clk/bcm/Makefile
@@ -10,3 +10,4 @@ obj-$(CONFIG_COMMON_CLK_IPROC)	+= clk-ns2.o
 obj-$(CONFIG_ARCH_BCM_CYGNUS)	+= clk-cygnus.o
 obj-$(CONFIG_ARCH_BCM_NSP)	+= clk-nsp.o
 obj-$(CONFIG_ARCH_BCM_5301X)	+= clk-nsp.o
+obj-$(CONFIG_ARCH_BCM_5301X)	+= clk-ns-ilp.o
diff --git a/drivers/clk/bcm/clk-ns-ilp.c b/drivers/clk/bcm/clk-ns-ilp.c
new file mode 100644
index 0000000..230458e8
--- /dev/null
+++ b/drivers/clk/bcm/clk-ns-ilp.c
@@ -0,0 +1,146 @@
+/*
+ * Copyright (C) 2016 Rafał Miłecki <rafal@milecki.pl>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/slab.h>
+
+#define PMU_XTAL_FREQ_RATIO			0x66c
+#define  XTAL_ALP_PER_4ILP			0x00001fff
+#define  XTAL_CTL_EN				0x80000000
+#define PMU_SLOW_CLK_PERIOD			0x6dc
+
+struct ns_ilp {
+	struct clk *clk;
+	struct clk_hw hw;
+	struct clk *alp_clk;
+	void __iomem *pmu;
+};
+
+static int ns_ilp_enable(struct clk_hw *hw)
+{
+	struct ns_ilp *ilp = container_of(hw, struct ns_ilp, hw);
+
+	writel(0x10199, ilp->pmu + PMU_SLOW_CLK_PERIOD);
+	writel(0x10000, ilp->pmu + 0x674);
+
+	return 0;
+}
+
+static unsigned long ns_ilp_recalc_rate(struct clk_hw *hw,
+					unsigned long parent_rate)
+{
+	struct ns_ilp *ilp = container_of(hw, struct ns_ilp, hw);
+	void __iomem *pmu = ilp->pmu;
+	u32 last_val, cur_val;
+	u32 sum = 0, num = 0, loop_num = 0;
+	u32 avg;
+	int err;
+
+	err = clk_prepare_enable(ilp->alp_clk);
+	if (err)
+		return 0;
+
+	/* Enable */
+	writel(XTAL_CTL_EN, pmu + PMU_XTAL_FREQ_RATIO);
+
+	/* Read initial value */
+	last_val = readl(pmu + PMU_XTAL_FREQ_RATIO) & XTAL_ALP_PER_4ILP;
+
+	/* Try getting 20 different values for calculating average */
+	while (num < 20) {
+		cur_val = readl(pmu + PMU_XTAL_FREQ_RATIO) & XTAL_ALP_PER_4ILP;
+
+		if (cur_val != last_val) {
+			/* Got different value, use it */
+			sum += cur_val;
+			num++;
+			loop_num = 0;
+			last_val = cur_val;
+		} else if (++loop_num > 5000) {
+			/* Same value over and over, give up */
+			sum += cur_val;
+			num++;
+			break;
+		}
+	}
+
+	/* Disable */
+	writel(0x0, pmu + PMU_XTAL_FREQ_RATIO);
+
+	avg = sum / num;
+
+	return clk_get_rate(ilp->alp_clk) * 4 / avg;
+}
+
+const struct clk_ops ns_ilp_clk_ops = {
+	.enable = ns_ilp_enable,
+	.recalc_rate = ns_ilp_recalc_rate,
+};
+
+static void ns_ilp_init(struct device_node *np)
+{
+	struct ns_ilp *ilp;
+	struct resource res;
+	struct clk_init_data init = { 0 };
+	int index;
+	int err;
+
+	ilp = kzalloc(sizeof(*ilp), GFP_KERNEL);
+	if (!ilp)
+		return;
+
+	index = of_property_match_string(np, "reg-names", "pmu");
+	if (index < 0) {
+		err = index;
+		goto err_free_ilp;
+	}
+	err = of_address_to_resource(np, index, &res);
+	if (err) {
+		err = index;
+		goto err_free_ilp;
+	}
+	ilp->pmu = ioremap_nocache(res.start, resource_size(&res));
+	if (IS_ERR(ilp->pmu)) {
+		err = PTR_ERR(ilp->pmu);
+		goto err_free_ilp;
+	}
+
+	ilp->alp_clk = of_clk_get_by_name(np, "alp-clk");
+	if (IS_ERR(ilp->alp_clk)) {
+		err = PTR_ERR(ilp->alp_clk);
+		goto err_unmap_pmu;
+	}
+
+	init.name = np->name;
+	init.ops = &ns_ilp_clk_ops;
+	init.flags = CLK_IS_ROOT;
+
+	ilp->hw.init = &init;
+	ilp->clk = clk_register(NULL, &ilp->hw);
+	if (WARN_ON(IS_ERR(ilp->clk)))
+		goto err_put_alp_clk;
+
+	of_clk_add_provider(np, of_clk_src_simple_get, ilp->clk);
+
+	return;
+
+err_put_alp_clk:
+	clk_put(ilp->alp_clk);
+err_unmap_pmu:
+	iounmap(ilp->pmu);
+err_free_ilp:
+	kfree(ilp);
+	pr_err("Failed to init ILP clock: %d\n", err);
+}
+CLK_OF_DECLARE(ns_ilp_clk, "brcm,ns-ilp", ns_ilp_init);
-- 
1.8.4.5

Re: [PATCH] clk: bcm: Add driver for Northstar ILP clock

[Mark Rutland]

On Fri, Jul 29, 2016 at 02:58:32PM +0200, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> This clock is present on cheaper Northstar devices like BCM53573 or
> BCM47189 using Corex-A7. This driver uses PMU (Power Management Unit)
> to calculate clock rate and allows using it in a generic (clk_*) way.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>  .../devicetree/bindings/clock/brcm,ns-ilp.txt      |  28 ++++
>  drivers/clk/bcm/Makefile                           |   1 +
>  drivers/clk/bcm/clk-ns-ilp.c                       | 146 +++++++++++++++++++++
>  3 files changed, 175 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt
>  create mode 100644 drivers/clk/bcm/clk-ns-ilp.c
> 
> diff --git a/Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt b/Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt
> new file mode 100644
> index 0000000..c4df38e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt
> @@ -0,0 +1,28 @@
> +Broadcom Northstar ILP clock
> +============================
> +
> +This binding uses the common clock binding:
> +    Documentation/devicetree/bindings/clock/clock-bindings.txt
> +
> +This binding is used for ILP clock on Broadcom Northstar devices using
> +Corex-A7 CPU. ILP clock depends on ALP one and has to be calculated on
> +runtime.
> +
> +Required properties:
> +- compatible: "brcm,ns-ilp"
> +- reg: iomem address range of PMU (Power Management Unit)
> +- reg-names: "pmu", the only needed & supported reg right now

>From the commit message and binding description, it sounds like there
should be a binding for the PMU, and that should cover the clocks
required/exported by the PMU.

> +- clocks: should reference an ALP clock
> +- clock-names: "alp", the only needed & supported clock right now
> +- #clock-cells: should be <0>

How many clocks does the PMU output, including the ILP clock?

> +static unsigned long ns_ilp_recalc_rate(struct clk_hw *hw,
> +					unsigned long parent_rate)
> +{
> +	struct ns_ilp *ilp = container_of(hw, struct ns_ilp, hw);
> +	void __iomem *pmu = ilp->pmu;
> +	u32 last_val, cur_val;
> +	u32 sum = 0, num = 0, loop_num = 0;
> +	u32 avg;
> +	int err;
> +
> +	err = clk_prepare_enable(ilp->alp_clk);
> +	if (err)
> +		return 0;
> +
> +	/* Enable */
> +	writel(XTAL_CTL_EN, pmu + PMU_XTAL_FREQ_RATIO);

What exactly are we enabling here?

> +
> +	/* Read initial value */
> +	last_val = readl(pmu + PMU_XTAL_FREQ_RATIO) & XTAL_ALP_PER_4ILP;
> +
> +	/* Try getting 20 different values for calculating average */

Please describe *why* this is necessary (i.e. why we need to calculate
an average).

> +	while (num < 20) {
> +		cur_val = readl(pmu + PMU_XTAL_FREQ_RATIO) & XTAL_ALP_PER_4ILP;
> +
> +		if (cur_val != last_val) {
> +			/* Got different value, use it */
> +			sum += cur_val;
> +			num++;
> +			loop_num = 0;
> +			last_val = cur_val;
> +		} else if (++loop_num > 5000) {
> +			/* Same value over and over, give up */
> +			sum += cur_val;
> +			num++;
> +			break;
> +		}
> +	}
> +
> +	/* Disable */
> +	writel(0x0, pmu + PMU_XTAL_FREQ_RATIO);
> +
> +	avg = sum / num;
> +
> +	return clk_get_rate(ilp->alp_clk) * 4 / avg;
> +}

Shouldn't the clk_prepare_enable be balanced?

> +	index = of_property_match_string(np, "reg-names", "pmu");
> +	if (index < 0) {
> +		err = index;
> +		goto err_free_ilp;
> +	}
> +	err = of_address_to_resource(np, index, &res);
> +	if (err) {
> +		err = index;
> +		goto err_free_ilp;
> +	}
> +	ilp->pmu = ioremap_nocache(res.start, resource_size(&res));

Use ioremap() for mapping devices.

It is not correct to use ioremap_nocache().

We should probably add an of_get_address_resource_by_name() or something
like that to avoid drivers having to open-code this kind of thing.

> +	if (IS_ERR(ilp->pmu)) {
> +		err = PTR_ERR(ilp->pmu);
> +		goto err_free_ilp;
> +	}
> +
> +	ilp->alp_clk = of_clk_get_by_name(np, "alp-clk");

The binding document said "alp", not "alp-clk".

We already know it's a clock, so the "-clk" suffix is redundant.

> +	if (IS_ERR(ilp->alp_clk)) {
> +		err = PTR_ERR(ilp->alp_clk);
> +		goto err_unmap_pmu;
> +	}
> +
> +	init.name = np->name;
> +	init.ops = &ns_ilp_clk_ops;
> +	init.flags = CLK_IS_ROOT;

That's not true; the ALP clock is a parent...

I thought CLK_IS_ROOT was disappearing, regardless.

Thanks,
Mark,

Re: [PATCH] clk: bcm: Add driver for Northstar ILP clock

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1622 bytes --]

Hi,

[auto build test ERROR on clk/clk-next]
[also build test ERROR on v4.7 next-20160729]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Rafa-Mi-ecki/clk-bcm-Add-driver-for-Northstar-ILP-clock/20160729-210140
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
config: arm-multi_v7_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   drivers/clk/bcm/clk-ns-ilp.c: In function 'ns_ilp_init':
>> drivers/clk/bcm/clk-ns-ilp.c:127:15: error: 'CLK_IS_ROOT' undeclared (first use in this function)
     init.flags = CLK_IS_ROOT;
                  ^
   drivers/clk/bcm/clk-ns-ilp.c:127:15: note: each undeclared identifier is reported only once for each function it appears in

vim +/CLK_IS_ROOT +127 drivers/clk/bcm/clk-ns-ilp.c

   121			err = PTR_ERR(ilp->alp_clk);
   122			goto err_unmap_pmu;
   123		}
   124	
   125		init.name = np->name;
   126		init.ops = &ns_ilp_clk_ops;
 > 127		init.flags = CLK_IS_ROOT;
   128	
   129		ilp->hw.init = &init;
   130		ilp->clk = clk_register(NULL, &ilp->hw);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 37739 bytes --]

Re: [PATCH] clk: bcm: Add driver for Northstar ILP clock

[Ray Jui]

Hi Rafal,

On 7/29/2016 5:58 AM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
>
> This clock is present on cheaper Northstar devices like BCM53573 or
> BCM47189 using Corex-A7. This driver uses PMU (Power Management Unit)
> to calculate clock rate and allows using it in a generic (clk_*) way.
>

I thought Northstar uses Cortex A9 instead of A7?

> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>  .../devicetree/bindings/clock/brcm,ns-ilp.txt      |  28 ++++
>  drivers/clk/bcm/Makefile                           |   1 +
>  drivers/clk/bcm/clk-ns-ilp.c                       | 146 +++++++++++++++++++++
>  3 files changed, 175 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt
>  create mode 100644 drivers/clk/bcm/clk-ns-ilp.c
>
> diff --git a/Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt b/Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt
> new file mode 100644
> index 0000000..c4df38e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt
> @@ -0,0 +1,28 @@
> +Broadcom Northstar ILP clock
> +============================
> +
> +This binding uses the common clock binding:
> +    Documentation/devicetree/bindings/clock/clock-bindings.txt
> +
> +This binding is used for ILP clock on Broadcom Northstar devices using
> +Corex-A7 CPU. ILP clock depends on ALP one and has to be calculated on
> +runtime.
> +

Cortex-A9.

> +Required properties:
> +- compatible: "brcm,ns-ilp"
> +- reg: iomem address range of PMU (Power Management Unit)
> +- reg-names: "pmu", the only needed & supported reg right now
> +- clocks: should reference an ALP clock
> +- clock-names: "alp", the only needed & supported clock right now

clock-names must be "alp-clk"

> +- #clock-cells: should be <0>
> +
> +Example:
> +
> +ilp: ilp {
> +	compatible = "brcm,ns-ilp";
> +	reg = <0x18012000 0x1000>;
> +	reg-names = "pmu";
> +	clocks = <&alp>;
> +	clock-names = "alp-clk";
> +	#clock-cells = <0>;
> +};
> diff --git a/drivers/clk/bcm/Makefile b/drivers/clk/bcm/Makefile
> index 1d79bd2..1389379 100644
> --- a/drivers/clk/bcm/Makefile
> +++ b/drivers/clk/bcm/Makefile
> @@ -10,3 +10,4 @@ obj-$(CONFIG_COMMON_CLK_IPROC)	+= clk-ns2.o
>  obj-$(CONFIG_ARCH_BCM_CYGNUS)	+= clk-cygnus.o
>  obj-$(CONFIG_ARCH_BCM_NSP)	+= clk-nsp.o
>  obj-$(CONFIG_ARCH_BCM_5301X)	+= clk-nsp.o
> +obj-$(CONFIG_ARCH_BCM_5301X)	+= clk-ns-ilp.o
> diff --git a/drivers/clk/bcm/clk-ns-ilp.c b/drivers/clk/bcm/clk-ns-ilp.c
> new file mode 100644
> index 0000000..230458e8
> --- /dev/null
> +++ b/drivers/clk/bcm/clk-ns-ilp.c
> @@ -0,0 +1,146 @@
> +/*
> + * Copyright (C) 2016 Rafał Miłecki <rafal@milecki.pl>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/slab.h>
> +
> +#define PMU_XTAL_FREQ_RATIO			0x66c
> +#define  XTAL_ALP_PER_4ILP			0x00001fff
> +#define  XTAL_CTL_EN				0x80000000
> +#define PMU_SLOW_CLK_PERIOD			0x6dc
> +
> +struct ns_ilp {
> +	struct clk *clk;
> +	struct clk_hw hw;
> +	struct clk *alp_clk;
> +	void __iomem *pmu;
> +};
> +
> +static int ns_ilp_enable(struct clk_hw *hw)
> +{
> +	struct ns_ilp *ilp = container_of(hw, struct ns_ilp, hw);
> +
> +	writel(0x10199, ilp->pmu + PMU_SLOW_CLK_PERIOD);
> +	writel(0x10000, ilp->pmu + 0x674);
> +
> +	return 0;
> +}
> +
> +static unsigned long ns_ilp_recalc_rate(struct clk_hw *hw,
> +					unsigned long parent_rate)
> +{
> +	struct ns_ilp *ilp = container_of(hw, struct ns_ilp, hw);
> +	void __iomem *pmu = ilp->pmu;
> +	u32 last_val, cur_val;
> +	u32 sum = 0, num = 0, loop_num = 0;
> +	u32 avg;
> +	int err;
> +
> +	err = clk_prepare_enable(ilp->alp_clk);
> +	if (err)
> +		return 0;
> +
> +	/* Enable */
> +	writel(XTAL_CTL_EN, pmu + PMU_XTAL_FREQ_RATIO);
> +
> +	/* Read initial value */
> +	last_val = readl(pmu + PMU_XTAL_FREQ_RATIO) & XTAL_ALP_PER_4ILP;
> +
> +	/* Try getting 20 different values for calculating average */
> +	while (num < 20) {
> +		cur_val = readl(pmu + PMU_XTAL_FREQ_RATIO) & XTAL_ALP_PER_4ILP;
> +
> +		if (cur_val != last_val) {
> +			/* Got different value, use it */
> +			sum += cur_val;
> +			num++;
> +			loop_num = 0;
> +			last_val = cur_val;
> +		} else if (++loop_num > 5000) {
> +			/* Same value over and over, give up */
> +			sum += cur_val;
> +			num++;
> +			break;
> +		}
> +	}
> +
> +	/* Disable */
> +	writel(0x0, pmu + PMU_XTAL_FREQ_RATIO);
> +
> +	avg = sum / num;
> +
> +	return clk_get_rate(ilp->alp_clk) * 4 / avg;
> +}
> +
> +const struct clk_ops ns_ilp_clk_ops = {

static

> +	.enable = ns_ilp_enable,
> +	.recalc_rate = ns_ilp_recalc_rate,
> +};
> +
> +static void ns_ilp_init(struct device_node *np)
> +{
> +	struct ns_ilp *ilp;
> +	struct resource res;
> +	struct clk_init_data init = { 0 };
> +	int index;
> +	int err;
> +
> +	ilp = kzalloc(sizeof(*ilp), GFP_KERNEL);
> +	if (!ilp)
> +		return;
> +
> +	index = of_property_match_string(np, "reg-names", "pmu");
> +	if (index < 0) {
> +		err = index;
> +		goto err_free_ilp;
> +	}
> +	err = of_address_to_resource(np, index, &res);
> +	if (err) {
> +		err = index;
> +		goto err_free_ilp;
> +	}
> +	ilp->pmu = ioremap_nocache(res.start, resource_size(&res));
> +	if (IS_ERR(ilp->pmu)) {
> +		err = PTR_ERR(ilp->pmu);
> +		goto err_free_ilp;
> +	}
> +
> +	ilp->alp_clk = of_clk_get_by_name(np, "alp-clk");
> +	if (IS_ERR(ilp->alp_clk)) {
> +		err = PTR_ERR(ilp->alp_clk);
> +		goto err_unmap_pmu;
> +	}
> +
> +	init.name = np->name;
> +	init.ops = &ns_ilp_clk_ops;
> +	init.flags = CLK_IS_ROOT;
> +
> +	ilp->hw.init = &init;
> +	ilp->clk = clk_register(NULL, &ilp->hw);
> +	if (WARN_ON(IS_ERR(ilp->clk)))
> +		goto err_put_alp_clk;
> +
> +	of_clk_add_provider(np, of_clk_src_simple_get, ilp->clk);

Check return value and handle error conditions

> +
> +	return;
> +
> +err_put_alp_clk:
> +	clk_put(ilp->alp_clk);
> +err_unmap_pmu:
> +	iounmap(ilp->pmu);
> +err_free_ilp:
> +	kfree(ilp);
> +	pr_err("Failed to init ILP clock: %d\n", err);
> +}
> +CLK_OF_DECLARE(ns_ilp_clk, "brcm,ns-ilp", ns_ilp_init);
>

Thanks,

Ray

Re: [PATCH] clk: bcm: Add driver for Northstar ILP clock

[Rafał Miłecki]

On 29 July 2016 at 22:44, Ray Jui <ray.jui@broadcom.com> wrote:
> On 7/29/2016 5:58 AM, Rafał Miłecki wrote:
>>
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> This clock is present on cheaper Northstar devices like BCM53573 or
>> BCM47189 using Corex-A7. This driver uses PMU (Power Management Unit)
>> to calculate clock rate and allows using it in a generic (clk_*) way.
>>
>
> I thought Northstar uses Cortex A9 instead of A7?

[    0.000000] CPU: ARMv7 Processor [410fc075] revision 5 (ARMv7), cr=10c5387d
[    0.000000] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing
instruction cache
[    0.000000] Machine model: Tenda AC9

-- 
Rafał

Re: [PATCH] clk: bcm: Add driver for Northstar ILP clock

[Ray Jui]

On 7/29/2016 1:46 PM, Rafał Miłecki wrote:
> On 29 July 2016 at 22:44, Ray Jui <ray.jui@broadcom.com> wrote:
>> On 7/29/2016 5:58 AM, Rafał Miłecki wrote:
>>>
>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>
>>> This clock is present on cheaper Northstar devices like BCM53573 or
>>> BCM47189 using Corex-A7. This driver uses PMU (Power Management Unit)
>>> to calculate clock rate and allows using it in a generic (clk_*) way.
>>>
>>
>> I thought Northstar uses Cortex A9 instead of A7?
>
> [    0.000000] CPU: ARMv7 Processor [410fc075] revision 5 (ARMv7), cr=10c5387d
> [    0.000000] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing
> instruction cache
> [    0.000000] Machine model: Tenda AC9
>

Yeah ARMv7 instruction set but the core is Cortex A7. Both Cortex A7 and 
A9 use ARMv7 instructions.

Thanks,

Ray

Re: [PATCH] clk: bcm: Add driver for Northstar ILP clock

[Rafał Miłecki]

On 29 July 2016 at 22:49, Ray Jui <ray.jui@broadcom.com> wrote:
> On 7/29/2016 1:46 PM, Rafał Miłecki wrote:
>> On 29 July 2016 at 22:44, Ray Jui <ray.jui@broadcom.com> wrote:
>>>
>>> On 7/29/2016 5:58 AM, Rafał Miłecki wrote:
>>>>
>>>>
>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>>
>>>> This clock is present on cheaper Northstar devices like BCM53573 or
>>>> BCM47189 using Corex-A7. This driver uses PMU (Power Management Unit)
>>>> to calculate clock rate and allows using it in a generic (clk_*) way.
>>>>
>>>
>>> I thought Northstar uses Cortex A9 instead of A7?
>>
>>
>> [    0.000000] CPU: ARMv7 Processor [410fc075] revision 5 (ARMv7),
>> cr=10c5387d
>> [    0.000000] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing
>> instruction cache
>> [    0.000000] Machine model: Tenda AC9
>>
>
> Yeah ARMv7 instruction set but the core is Cortex A7. Both Cortex A7 and A9
> use ARMv7 instructions.

OK, sorry for irrelevant part then :)

This is from BCM4709C0:
bcma: bus0: Core 10 found: ARM Cortex A9 core (ihost) (manuf 0x4BF, id
0x510, rev 0x07, class 0x0)

This is from BCM47189B0::
bcma: bus0: Core 3 found: ARM CA7 (manuf 0x4BF, id 0x847, rev 0x00, class 0x0)

-- 
Rafał

Re: [PATCH] clk: bcm: Add driver for Northstar ILP clock

[Florian Fainelli]

On 07/29/2016 01:52 PM, Rafał Miłecki wrote:
> On 29 July 2016 at 22:49, Ray Jui <ray.jui@broadcom.com> wrote:
>> On 7/29/2016 1:46 PM, Rafał Miłecki wrote:
>>> On 29 July 2016 at 22:44, Ray Jui <ray.jui@broadcom.com> wrote:
>>>>
>>>> On 7/29/2016 5:58 AM, Rafał Miłecki wrote:
>>>>>
>>>>>
>>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>>>
>>>>> This clock is present on cheaper Northstar devices like BCM53573 or
>>>>> BCM47189 using Corex-A7. This driver uses PMU (Power Management Unit)
>>>>> to calculate clock rate and allows using it in a generic (clk_*) way.
>>>>>
>>>>
>>>> I thought Northstar uses Cortex A9 instead of A7?
>>>
>>>
>>> [    0.000000] CPU: ARMv7 Processor [410fc075] revision 5 (ARMv7),
>>> cr=10c5387d
>>> [    0.000000] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing
>>> instruction cache
>>> [    0.000000] Machine model: Tenda AC9
>>>
>>
>> Yeah ARMv7 instruction set but the core is Cortex A7. Both Cortex A7 and A9
>> use ARMv7 instructions.
> 
> OK, sorry for irrelevant part then :)
> 
> This is from BCM4709C0:
> bcma: bus0: Core 10 found: ARM Cortex A9 core (ihost) (manuf 0x4BF, id
> 0x510, rev 0x07, class 0x0)
> 
> This is from BCM47189B0::
> bcma: bus0: Core 3 found: ARM CA7 (manuf 0x4BF, id 0x847, rev 0x00, class 0x0)
> 

This is indeed a Cortex A7-based chip, not clear if putting this chip in
the Northstar family is accurate here because it really seems to have a
different architecture from the NS/NSP family here...
-- 
Florian

Re: [PATCH] clk: bcm: Add driver for Northstar ILP clock

[Ray Jui]

On 7/29/2016 1:55 PM, Florian Fainelli wrote:
> On 07/29/2016 01:52 PM, Rafał Miłecki wrote:
>> On 29 July 2016 at 22:49, Ray Jui <ray.jui@broadcom.com> wrote:
>>> On 7/29/2016 1:46 PM, Rafał Miłecki wrote:
>>>> On 29 July 2016 at 22:44, Ray Jui <ray.jui@broadcom.com> wrote:
>>>>>
>>>>> On 7/29/2016 5:58 AM, Rafał Miłecki wrote:
>>>>>>
>>>>>>
>>>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>>>>
>>>>>> This clock is present on cheaper Northstar devices like BCM53573 or
>>>>>> BCM47189 using Corex-A7. This driver uses PMU (Power Management Unit)
>>>>>> to calculate clock rate and allows using it in a generic (clk_*) way.
>>>>>>
>>>>>
>>>>> I thought Northstar uses Cortex A9 instead of A7?
>>>>
>>>>
>>>> [    0.000000] CPU: ARMv7 Processor [410fc075] revision 5 (ARMv7),
>>>> cr=10c5387d
>>>> [    0.000000] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing
>>>> instruction cache
>>>> [    0.000000] Machine model: Tenda AC9
>>>>
>>>
>>> Yeah ARMv7 instruction set but the core is Cortex A7. Both Cortex A7 and A9
>>> use ARMv7 instructions.
>>
>> OK, sorry for irrelevant part then :)
>>
>> This is from BCM4709C0:
>> bcma: bus0: Core 10 found: ARM Cortex A9 core (ihost) (manuf 0x4BF, id
>> 0x510, rev 0x07, class 0x0)
>>
>> This is from BCM47189B0::
>> bcma: bus0: Core 3 found: ARM CA7 (manuf 0x4BF, id 0x847, rev 0x00, class 0x0)
>>
>
> This is indeed a Cortex A7-based chip, not clear if putting this chip in
> the Northstar family is accurate here because it really seems to have a
> different architecture from the NS/NSP family here...
>

Okay I got it. Good to know! I got confused by it being called 
"Northstar" because as far I can remember, none of the Northstar chips 
uses Cortex A7 (or maybe even that assumption is incorrect, :))

Thanks,

Ray

Re: [PATCH] clk: bcm: Add driver for Northstar ILP clock

[Rafał Miłecki]

On 29 July 2016 at 22:55, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 07/29/2016 01:52 PM, Rafał Miłecki wrote:
>> On 29 July 2016 at 22:49, Ray Jui <ray.jui@broadcom.com> wrote:
>>> On 7/29/2016 1:46 PM, Rafał Miłecki wrote:
>>>> On 29 July 2016 at 22:44, Ray Jui <ray.jui@broadcom.com> wrote:
>>>>>
>>>>> On 7/29/2016 5:58 AM, Rafał Miłecki wrote:
>>>>>>
>>>>>>
>>>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>>>>
>>>>>> This clock is present on cheaper Northstar devices like BCM53573 or
>>>>>> BCM47189 using Corex-A7. This driver uses PMU (Power Management Unit)
>>>>>> to calculate clock rate and allows using it in a generic (clk_*) way.
>>>>>>
>>>>>
>>>>> I thought Northstar uses Cortex A9 instead of A7?
>>>>
>>>>
>>>> [    0.000000] CPU: ARMv7 Processor [410fc075] revision 5 (ARMv7),
>>>> cr=10c5387d
>>>> [    0.000000] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing
>>>> instruction cache
>>>> [    0.000000] Machine model: Tenda AC9
>>>>
>>>
>>> Yeah ARMv7 instruction set but the core is Cortex A7. Both Cortex A7 and A9
>>> use ARMv7 instructions.
>>
>> OK, sorry for irrelevant part then :)
>>
>> This is from BCM4709C0:
>> bcma: bus0: Core 10 found: ARM Cortex A9 core (ihost) (manuf 0x4BF, id
>> 0x510, rev 0x07, class 0x0)
>>
>> This is from BCM47189B0::
>> bcma: bus0: Core 3 found: ARM CA7 (manuf 0x4BF, id 0x847, rev 0x00, class 0x0)
>>
>
> This is indeed a Cortex A7-based chip, not clear if putting this chip in
> the Northstar family is accurate here because it really seems to have a
> different architecture from the NS/NSP family here...

Broadcom claims it is a Northstar, at least according to their SDKs:

Asus RT-AC1200G+

# cat /proc/cpuinfo
Processor : ARMv7 Processor rev 5 (v7l)
BogoMIPS : 1795.68
Features : swp half thumb fastmult edsp
CPU implementer : 0x41
CPU architecture: 7
CPU variant : 0x0
CPU part : 0xc07
CPU revision : 5

Hardware : Northstar Prototype
Revision : 0000
Serial : 0000000000000000

-- 
Rafał

Re: [PATCH] clk: bcm: Add driver for Northstar ILP clock

[Rafał Miłecki]

On 29 July 2016 at 15:15, Mark Rutland <mark.rutland@arm.com> wrote:
> On Fri, Jul 29, 2016 at 02:58:32PM +0200, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> This clock is present on cheaper Northstar devices like BCM53573 or
>> BCM47189 using Corex-A7. This driver uses PMU (Power Management Unit)
>> to calculate clock rate and allows using it in a generic (clk_*) way.
>>
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> ---
>>  .../devicetree/bindings/clock/brcm,ns-ilp.txt      |  28 ++++
>>  drivers/clk/bcm/Makefile                           |   1 +
>>  drivers/clk/bcm/clk-ns-ilp.c                       | 146 +++++++++++++++++++++
>>  3 files changed, 175 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt
>>  create mode 100644 drivers/clk/bcm/clk-ns-ilp.c
>>
>> diff --git a/Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt b/Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt
>> new file mode 100644
>> index 0000000..c4df38e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt
>> @@ -0,0 +1,28 @@
>> +Broadcom Northstar ILP clock
>> +============================
>> +
>> +This binding uses the common clock binding:
>> +    Documentation/devicetree/bindings/clock/clock-bindings.txt
>> +
>> +This binding is used for ILP clock on Broadcom Northstar devices using
>> +Corex-A7 CPU. ILP clock depends on ALP one and has to be calculated on
>> +runtime.
>> +
>> +Required properties:
>> +- compatible: "brcm,ns-ilp"
>> +- reg: iomem address range of PMU (Power Management Unit)
>> +- reg-names: "pmu", the only needed & supported reg right now
>
> From the commit message and binding description, it sounds like there
> should be a binding for the PMU, and that should cover the clocks
> required/exported by the PMU.

This is a bit of problem, because PMU handles a lot of different stuff
and is used by various drivers. Some examples of what you can do
with/find on a PMU:
1) Power management
2) Watchdog
3) Timer
4) XTAL
5) PLLs
6) Control registers for some ARM debugging (whatever it is), UART, JTAG, more

PMU is used by different drivers, e.g.:
1) Ethernet driver
2) Wireless driver
3) NAND controller driver

I don't have access to Broadcom's datasheets so unfortunately I can't
provide all details.


>> +- clocks: should reference an ALP clock
>> +- clock-names: "alp", the only needed & supported clock right now
>> +- #clock-cells: should be <0>
>
> How many clocks does the PMU output, including the ILP clock?

Well, ALP clock (AKA XTAL clock) is definitely part of PMU. It's a
fixed rate clock with rate specific to the chip.

I think ILP is also part of PMU (again: I don't have datasheets) as
PMU has this ALP_PER_4ILP register.

>From Broadcom's SDK I can say they also have "ARM debug unit" on some
chipsets. It requires enabling "ARM debug clk" to operate which is
handled by PMU as well.

-- 
Rafał

[PATCH V2] clk: bcm: Add driver for Northstar ILP clock

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

This clock is present on cheaper Northstar devices like BCM53573 or
BCM47189 using Corex-A7. This driver uses PMU (Power Management Unit)
to calculate clock rate and allows using it in a generic (clk_*) way.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V2: Rebase on top of clk-next
    Use ALP as parent clock
    Improve comments
    Switch from ioremap_nocache to ioremap
    Check of_clk_add_provide result for error
---
 .../devicetree/bindings/clock/brcm,ns-ilp.txt      |  26 ++++
 drivers/clk/bcm/Makefile                           |   1 +
 drivers/clk/bcm/clk-ns-ilp.c                       | 147 +++++++++++++++++++++
 3 files changed, 174 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt
 create mode 100644 drivers/clk/bcm/clk-ns-ilp.c

diff --git a/Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt b/Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt
new file mode 100644
index 0000000..2c862a0
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt
@@ -0,0 +1,26 @@
+Broadcom Northstar ILP clock
+============================
+
+This binding uses the common clock binding:
+    Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+This binding is used for ILP clock on Broadcom Northstar devices using
+Corex-A7 CPU. ILP clock depends on ALP one and has to be calculated on
+runtime.
+
+Required properties:
+- compatible: "brcm,ns-ilp"
+- reg: iomem address range of PMU (Power Management Unit)
+- reg-names: "pmu", the only needed & supported reg right now
+- clocks: has to reference an ALP clock
+- #clock-cells: should be <0>
+
+Example:
+
+ilp: ilp {
+	compatible = "brcm,ns-ilp";
+	reg = <0x18012000 0x1000>;
+	reg-names = "pmu";
+	clocks = <&alp>;
+	#clock-cells = <0>;
+};
diff --git a/drivers/clk/bcm/Makefile b/drivers/clk/bcm/Makefile
index 1d79bd2..1389379 100644
--- a/drivers/clk/bcm/Makefile
+++ b/drivers/clk/bcm/Makefile
@@ -10,3 +10,4 @@ obj-$(CONFIG_COMMON_CLK_IPROC)	+= clk-ns2.o
 obj-$(CONFIG_ARCH_BCM_CYGNUS)	+= clk-cygnus.o
 obj-$(CONFIG_ARCH_BCM_NSP)	+= clk-nsp.o
 obj-$(CONFIG_ARCH_BCM_5301X)	+= clk-nsp.o
+obj-$(CONFIG_ARCH_BCM_5301X)	+= clk-ns-ilp.o
diff --git a/drivers/clk/bcm/clk-ns-ilp.c b/drivers/clk/bcm/clk-ns-ilp.c
new file mode 100644
index 0000000..0337313
--- /dev/null
+++ b/drivers/clk/bcm/clk-ns-ilp.c
@@ -0,0 +1,147 @@
+/*
+ * Copyright (C) 2016 Rafał Miłecki <rafal@milecki.pl>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/slab.h>
+
+#define PMU_XTAL_FREQ_RATIO			0x66c
+#define  XTAL_ALP_PER_4ILP			0x00001fff
+#define  XTAL_CTL_EN				0x80000000
+#define PMU_SLOW_CLK_PERIOD			0x6dc
+
+struct ns_ilp {
+	struct clk *clk;
+	struct clk_hw hw;
+	void __iomem *pmu;
+};
+
+static int ns_ilp_enable(struct clk_hw *hw)
+{
+	struct ns_ilp *ilp = container_of(hw, struct ns_ilp, hw);
+
+	writel(0x10199, ilp->pmu + PMU_SLOW_CLK_PERIOD);
+	writel(0x10000, ilp->pmu + 0x674);
+
+	return 0;
+}
+
+static unsigned long ns_ilp_recalc_rate(struct clk_hw *hw,
+					unsigned long parent_rate)
+{
+	struct ns_ilp *ilp = container_of(hw, struct ns_ilp, hw);
+	void __iomem *pmu = ilp->pmu;
+	u32 last_val, cur_val;
+	u32 sum = 0, num = 0, loop_num = 0;
+	u32 avg;
+
+	/* Enable measurement */
+	writel(XTAL_CTL_EN, pmu + PMU_XTAL_FREQ_RATIO);
+
+	/* Read initial value */
+	last_val = readl(pmu + PMU_XTAL_FREQ_RATIO) & XTAL_ALP_PER_4ILP;
+
+	/*
+	 * At minimum we should loop for a bit to let hardware do the
+	 * measurement. This isn't very accurate however, so for a better
+	 * precision lets try getting 20 different values for and use average.
+	 */
+	while (num < 20) {
+		cur_val = readl(pmu + PMU_XTAL_FREQ_RATIO) & XTAL_ALP_PER_4ILP;
+
+		if (cur_val != last_val) {
+			/* Got different value, use it */
+			sum += cur_val;
+			num++;
+			loop_num = 0;
+			last_val = cur_val;
+		} else if (++loop_num > 5000) {
+			/* Same value over and over, give up */
+			sum += cur_val;
+			num++;
+			break;
+		}
+	}
+
+	/* Disable measurement to save power */
+	writel(0x0, pmu + PMU_XTAL_FREQ_RATIO);
+
+	avg = sum / num;
+
+	return parent_rate * 4 / avg;
+}
+
+static const struct clk_ops ns_ilp_clk_ops = {
+	.enable = ns_ilp_enable,
+	.recalc_rate = ns_ilp_recalc_rate,
+};
+
+static void ns_ilp_init(struct device_node *np)
+{
+	struct ns_ilp *ilp;
+	struct resource res;
+	struct clk_init_data init = { 0 };
+	const char *parent_name;
+	int index;
+	int err;
+
+	ilp = kzalloc(sizeof(*ilp), GFP_KERNEL);
+	if (!ilp)
+		return;
+
+	parent_name = of_clk_get_parent_name(np, 0);
+	if (!parent_name) {
+		err = -ENOENT;
+		goto err_free_ilp;
+	}
+
+	/* TODO: This looks generic, try making it OF helper. */
+	index = of_property_match_string(np, "reg-names", "pmu");
+	if (index < 0) {
+		err = index;
+		goto err_free_ilp;
+	}
+	err = of_address_to_resource(np, index, &res);
+	if (err)
+		goto err_free_ilp;
+	ilp->pmu = ioremap(res.start, resource_size(&res));
+	if (IS_ERR(ilp->pmu)) {
+		err = PTR_ERR(ilp->pmu);
+		goto err_free_ilp;
+	}
+
+	init.name = np->name;
+	init.ops = &ns_ilp_clk_ops;
+	init.parent_names = &parent_name;
+	init.num_parents = 1;
+
+	ilp->hw.init = &init;
+	ilp->clk = clk_register(NULL, &ilp->hw);
+	if (WARN_ON(IS_ERR(ilp->clk)))
+		goto err_unmap_pmu;
+
+	err = of_clk_add_provider(np, of_clk_src_simple_get, ilp->clk);
+	if (err)
+		goto err_clk_unregister;
+
+	return;
+
+err_clk_unregister:
+	clk_unregister(ilp->clk);
+err_unmap_pmu:
+	iounmap(ilp->pmu);
+err_free_ilp:
+	kfree(ilp);
+	pr_err("Failed to init ILP clock: %d\n", err);
+}
+CLK_OF_DECLARE(ns_ilp_clk, "brcm,ns-ilp", ns_ilp_init);
-- 
1.8.4.5

Re: [PATCH V2] clk: bcm: Add driver for Northstar ILP clock

[Paul Gortmaker]

On Fri, Jul 29, 2016 at 6:45 PM, Rafał Miłecki <zajec5@gmail.com> wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
>
> This clock is present on cheaper Northstar devices like BCM53573 or
> BCM47189 using Corex-A7. This driver uses PMU (Power Management Unit)
> to calculate clock rate and allows using it in a generic (clk_*) way.
>
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> V2: Rebase on top of clk-next
>     Use ALP as parent clock
>     Improve comments
>     Switch from ioremap_nocache to ioremap
>     Check of_clk_add_provide result for error
> ---
>  .../devicetree/bindings/clock/brcm,ns-ilp.txt      |  26 ++++
>  drivers/clk/bcm/Makefile                           |   1 +
>  drivers/clk/bcm/clk-ns-ilp.c                       | 147 +++++++++++++++++++++
>  3 files changed, 174 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt
>  create mode 100644 drivers/clk/bcm/clk-ns-ilp.c
>
> diff --git a/Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt b/Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt
> new file mode 100644
> index 0000000..2c862a0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt
> @@ -0,0 +1,26 @@
> +Broadcom Northstar ILP clock
> +============================
> +
> +This binding uses the common clock binding:
> +    Documentation/devicetree/bindings/clock/clock-bindings.txt
> +
> +This binding is used for ILP clock on Broadcom Northstar devices using
> +Corex-A7 CPU. ILP clock depends on ALP one and has to be calculated on
> +runtime.
> +
> +Required properties:
> +- compatible: "brcm,ns-ilp"
> +- reg: iomem address range of PMU (Power Management Unit)
> +- reg-names: "pmu", the only needed & supported reg right now
> +- clocks: has to reference an ALP clock
> +- #clock-cells: should be <0>
> +
> +Example:
> +
> +ilp: ilp {
> +       compatible = "brcm,ns-ilp";
> +       reg = <0x18012000 0x1000>;
> +       reg-names = "pmu";
> +       clocks = <&alp>;
> +       #clock-cells = <0>;
> +};
> diff --git a/drivers/clk/bcm/Makefile b/drivers/clk/bcm/Makefile
> index 1d79bd2..1389379 100644
> --- a/drivers/clk/bcm/Makefile
> +++ b/drivers/clk/bcm/Makefile
> @@ -10,3 +10,4 @@ obj-$(CONFIG_COMMON_CLK_IPROC)        += clk-ns2.o
>  obj-$(CONFIG_ARCH_BCM_CYGNUS)  += clk-cygnus.o
>  obj-$(CONFIG_ARCH_BCM_NSP)     += clk-nsp.o
>  obj-$(CONFIG_ARCH_BCM_5301X)   += clk-nsp.o
> +obj-$(CONFIG_ARCH_BCM_5301X)   += clk-ns-ilp.o
> diff --git a/drivers/clk/bcm/clk-ns-ilp.c b/drivers/clk/bcm/clk-ns-ilp.c
> new file mode 100644
> index 0000000..0337313
> --- /dev/null
> +++ b/drivers/clk/bcm/clk-ns-ilp.c
> @@ -0,0 +1,147 @@
> +/*
> + * Copyright (C) 2016 Rafał Miłecki <rafal@milecki.pl>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>

I didn't see anything using modular content from the above header file, so
I think you can remove it.

Paul.
--

> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/slab.h>
> +
> +#define PMU_XTAL_FREQ_RATIO                    0x66c
> +#define  XTAL_ALP_PER_4ILP                     0x00001fff
> +#define  XTAL_CTL_EN                           0x80000000
> +#define PMU_SLOW_CLK_PERIOD                    0x6dc
> +

Re: [PATCH] clk: bcm: Add driver for Northstar ILP clock

[Mark Rutland]

On Fri, Jul 29, 2016 at 11:24:50PM +0200, Rafał Miłecki wrote:
> On 29 July 2016 at 15:15, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Fri, Jul 29, 2016 at 02:58:32PM +0200, Rafał Miłecki wrote:
> >> From: Rafał Miłecki <rafal@milecki.pl>
> >>
> >> This clock is present on cheaper Northstar devices like BCM53573 or
> >> BCM47189 using Corex-A7. This driver uses PMU (Power Management Unit)
> >> to calculate clock rate and allows using it in a generic (clk_*) way.
> >>
> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> >> ---
> >>  .../devicetree/bindings/clock/brcm,ns-ilp.txt      |  28 ++++
> >>  drivers/clk/bcm/Makefile                           |   1 +
> >>  drivers/clk/bcm/clk-ns-ilp.c                       | 146 +++++++++++++++++++++
> >>  3 files changed, 175 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt
> >>  create mode 100644 drivers/clk/bcm/clk-ns-ilp.c
> >>
> >> diff --git a/Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt b/Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt
> >> new file mode 100644
> >> index 0000000..c4df38e
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt
> >> @@ -0,0 +1,28 @@
> >> +Broadcom Northstar ILP clock
> >> +============================
> >> +
> >> +This binding uses the common clock binding:
> >> +    Documentation/devicetree/bindings/clock/clock-bindings.txt
> >> +
> >> +This binding is used for ILP clock on Broadcom Northstar devices using
> >> +Corex-A7 CPU. ILP clock depends on ALP one and has to be calculated on
> >> +runtime.
> >> +
> >> +Required properties:
> >> +- compatible: "brcm,ns-ilp"
> >> +- reg: iomem address range of PMU (Power Management Unit)
> >> +- reg-names: "pmu", the only needed & supported reg right now
> >
> > From the commit message and binding description, it sounds like there
> > should be a binding for the PMU, and that should cover the clocks
> > required/exported by the PMU.
> 
> This is a bit of problem, because PMU handles a lot of different stuff
> and is used by various drivers. Some examples of what you can do
> with/find on a PMU:
> 1) Power management
> 2) Watchdog
> 3) Timer
> 4) XTAL
> 5) PLLs
> 6) Control registers for some ARM debugging (whatever it is), UART, JTAG, more

The organisation of subsystems in Libnux shouldn't affect the binding in
this manner.

There'll likely be shared resources (e.g. register ranges), and you need
knowledge of the entire PMU to operate the sub-resources (and their
potentially shared dependencies) without conflict.

While you might one sub-nodes on a larger PMU node, these should
certainly not be entirely separate nodes and/or bindings.

> >> +- clocks: should reference an ALP clock
> >> +- clock-names: "alp", the only needed & supported clock right now
> >> +- #clock-cells: should be <0>
> >
> > How many clocks does the PMU output, including the ILP clock?
> 
> Well, ALP clock (AKA XTAL clock) is definitely part of PMU. It's a
> fixed rate clock with rate specific to the chip.
> 
> I think ILP is also part of PMU (again: I don't have datasheets) as
> PMU has this ALP_PER_4ILP register.

Ok. Is that output from the PMU, or is it strictly internal?

> From Broadcom's SDK I can say they also have "ARM debug unit" on some
> chipsets. It requires enabling "ARM debug clk" to operate which is
> handled by PMU as well.

Likewise?

Thanks,
Mark.

Re: [PATCH V2] clk: bcm: Add driver for Northstar ILP clock

[Rob Herring]

On Sat, Jul 30, 2016 at 12:45:40AM +0200, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> This clock is present on cheaper Northstar devices like BCM53573 or
> BCM47189 using Corex-A7. This driver uses PMU (Power Management Unit)
> to calculate clock rate and allows using it in a generic (clk_*) way.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> V2: Rebase on top of clk-next
>     Use ALP as parent clock
>     Improve comments
>     Switch from ioremap_nocache to ioremap
>     Check of_clk_add_provide result for error
> ---
>  .../devicetree/bindings/clock/brcm,ns-ilp.txt      |  26 ++++
>  drivers/clk/bcm/Makefile                           |   1 +
>  drivers/clk/bcm/clk-ns-ilp.c                       | 147 +++++++++++++++++++++
>  3 files changed, 174 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt
>  create mode 100644 drivers/clk/bcm/clk-ns-ilp.c
> 
> diff --git a/Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt b/Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt
> new file mode 100644
> index 0000000..2c862a0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt
> @@ -0,0 +1,26 @@
> +Broadcom Northstar ILP clock
> +============================
> +
> +This binding uses the common clock binding:
> +    Documentation/devicetree/bindings/clock/clock-bindings.txt
> +
> +This binding is used for ILP clock on Broadcom Northstar devices using
> +Corex-A7 CPU. ILP clock depends on ALP one and has to be calculated on
> +runtime.
> +
> +Required properties:
> +- compatible: "brcm,ns-ilp"
> +- reg: iomem address range of PMU (Power Management Unit)

PMU would imply to me there is more than just 1 clock the block 
controls and the compatible should reflect that if so.

> +- reg-names: "pmu", the only needed & supported reg right now
> +- clocks: has to reference an ALP clock
> +- #clock-cells: should be <0>
> +
> +Example:
> +
> +ilp: ilp {

ilp@18012000

> +	compatible = "brcm,ns-ilp";
> +	reg = <0x18012000 0x1000>;
> +	reg-names = "pmu";
> +	clocks = <&alp>;
> +	#clock-cells = <0>;
> +};