[PATCH v3 16/18] dt-bindings: Document img,boston-clock binding

[PATCH v3 16/18] dt-bindings: Document img,boston-clock binding

[Paul Burton]

Add device tree binding documentation for the clocks provided by the
MIPS Boston development board from Imagination Technologies, and a
header file describing the available clocks for use by device trees &
driver.

Signed-off-by: Paul Burton <paul.burton@imgtec.com>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: linux-clk@vger.kernel.org
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: devicetree@vger.kernel.org

---

Changes in v3: None
Changes in v2:
- Add BOSTON_CLK_INPUT to expose the input clock.

 .../devicetree/bindings/clock/img,boston-clock.txt | 27 ++++++++++++++++++++++
 include/dt-bindings/clock/boston-clock.h           | 14 +++++++++++
 2 files changed, 41 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/img,boston-clock.txt
 create mode 100644 include/dt-bindings/clock/boston-clock.h

diff --git a/Documentation/devicetree/bindings/clock/img,boston-clock.txt b/Documentation/devicetree/bindings/clock/img,boston-clock.txt
new file mode 100644
index 0000000..c01ea60
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/img,boston-clock.txt
@@ -0,0 +1,27 @@
+Binding for Imagination Technologies MIPS Boston clock sources.
+
+This binding uses the common clock binding[1].
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible : Should be "img,boston-clock".
+- #clock-cells : Should be set to 1.
+  Values available for clock consumers can be found in the header file:
+    <dt-bindings/clock/boston-clock.h>
+- regmap : Phandle to the Boston platform register system controller.
+  This should contain a phandle to the system controller node covering the
+  platform registers provided by the Boston board.
+
+Example:
+
+	clk_boston: clock {
+		compatible = "img,boston-clock";
+		#clock-cells = <1>;
+		regmap = <&plat_regs>;
+	};
+
+	uart0: uart@17ffe000 {
+		/* ... */
+		clocks = <&clk_boston BOSTON_CLK_SYS>;
+	};
diff --git a/include/dt-bindings/clock/boston-clock.h b/include/dt-bindings/clock/boston-clock.h
new file mode 100644
index 0000000..a6f0098
--- /dev/null
+++ b/include/dt-bindings/clock/boston-clock.h
@@ -0,0 +1,14 @@
+/*
+ * Copyright (C) 2016 Imagination Technologies
+ *
+ * SPDX-License-Identifier:	GPL-2.0
+ */
+
+#ifndef __DT_BINDINGS_CLOCK_BOSTON_CLOCK_H__
+#define __DT_BINDINGS_CLOCK_BOSTON_CLOCK_H__
+
+#define BOSTON_CLK_INPUT 0
+#define BOSTON_CLK_SYS 1
+#define BOSTON_CLK_CPU 2
+
+#endif /* __DT_BINDINGS_CLOCK_BOSTON_CLOCK_H__ */
-- 
2.10.0

[PATCH v3 17/18] clk: boston: Add a driver for MIPS Boston board clocks

[Paul Burton]

Add a driver for the clocks provided by the MIPS Boston board from
Imagination Technologies. 2 clocks are provided - the system clock & the
CPU clock - and each is a simple fixed rate clock whose frequency can be
determined by reading a register provided by the board.

Signed-off-by: Paul Burton <paul.burton@imgtec.com>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: linux-clk@vger.kernel.org

---

Changes in v3: None
Changes in v2:
- Support BOSTON_CLK_INPUT.
- Register clocks with clk_register_fixed_rate during boot, removing need for clk_ops.
- s/uint32_t/u32/.
- Move driver to a vendor directory.

 drivers/clk/Kconfig             |   1 +
 drivers/clk/Makefile            |   1 +
 drivers/clk/imgtec/Kconfig      |  10 ++++
 drivers/clk/imgtec/Makefile     |   1 +
 drivers/clk/imgtec/clk-boston.c | 101 ++++++++++++++++++++++++++++++++++++++++
 5 files changed, 114 insertions(+)
 create mode 100644 drivers/clk/imgtec/Kconfig
 create mode 100644 drivers/clk/imgtec/Makefile
 create mode 100644 drivers/clk/imgtec/clk-boston.c

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index e2d9bd7..a6c7e03 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -210,6 +210,7 @@ config COMMON_CLK_OXNAS
 
 source "drivers/clk/bcm/Kconfig"
 source "drivers/clk/hisilicon/Kconfig"
+source "drivers/clk/imgtec/Kconfig"
 source "drivers/clk/meson/Kconfig"
 source "drivers/clk/mvebu/Kconfig"
 source "drivers/clk/qcom/Kconfig"
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 3b6f9cf..8c4c425 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -60,6 +60,7 @@ obj-y					+= bcm/
 obj-$(CONFIG_ARCH_BERLIN)		+= berlin/
 obj-$(CONFIG_H8300)			+= h8300/
 obj-$(CONFIG_ARCH_HISI)			+= hisilicon/
+obj-y					+= imgtec/
 obj-$(CONFIG_ARCH_MXC)			+= imx/
 obj-$(CONFIG_MACH_INGENIC)		+= ingenic/
 obj-$(CONFIG_COMMON_CLK_KEYSTONE)	+= keystone/
diff --git a/drivers/clk/imgtec/Kconfig b/drivers/clk/imgtec/Kconfig
new file mode 100644
index 0000000..c2ea745
--- /dev/null
+++ b/drivers/clk/imgtec/Kconfig
@@ -0,0 +1,10 @@
+config COMMON_CLK_BOSTON
+	bool "Clock driver for MIPS Boston boards"
+	depends on MIPS || COMPILE_TEST
+	depends on OF
+	select MFD_SYSCON
+	---help---
+	  Enable this to support the system & CPU clocks on the MIPS Boston
+	  development board from Imagination Technologies. These are simple
+	  fixed rate clocks whose rate is determined by reading a platform
+	  provided register.
diff --git a/drivers/clk/imgtec/Makefile b/drivers/clk/imgtec/Makefile
new file mode 100644
index 0000000..ac779b8
--- /dev/null
+++ b/drivers/clk/imgtec/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_COMMON_CLK_BOSTON)		+= clk-boston.o
diff --git a/drivers/clk/imgtec/clk-boston.c b/drivers/clk/imgtec/clk-boston.c
new file mode 100644
index 0000000..14795b8
--- /dev/null
+++ b/drivers/clk/imgtec/clk-boston.c
@@ -0,0 +1,101 @@
+/*
+ * Copyright (C) 2016 Imagination Technologies
+ * Author: Paul Burton <paul.burton@imgtec.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/mfd/syscon.h>
+
+#include <dt-bindings/clock/boston-clock.h>
+
+#define BOSTON_PLAT_MMCMDIV		0x30
+# define BOSTON_PLAT_MMCMDIV_CLK0DIV	(0xff << 0)
+# define BOSTON_PLAT_MMCMDIV_INPUT	(0xff << 8)
+# define BOSTON_PLAT_MMCMDIV_MUL	(0xff << 16)
+# define BOSTON_PLAT_MMCMDIV_CLK1DIV	(0xff << 24)
+
+#define BOSTON_CLK_COUNT 3
+
+struct clk_boston_state {
+	struct clk *clks[BOSTON_CLK_COUNT];
+	struct clk_onecell_data onecell_data;
+};
+
+static u32 ext_field(u32 val, u32 mask)
+{
+	return (val & mask) >> (ffs(mask) - 1);
+}
+
+static void __init clk_boston_setup(struct device_node *np)
+{
+	unsigned long in_freq, cpu_freq, sys_freq;
+	uint mmcmdiv, mul, cpu_div, sys_div;
+	struct clk_boston_state *state;
+	struct regmap *regmap;
+	struct clk *clk;
+	int err;
+
+	regmap = syscon_regmap_lookup_by_phandle(np, "regmap");
+	if (IS_ERR(regmap)) {
+		pr_err("failed to find regmap\n");
+		return;
+	}
+
+	err = regmap_read(regmap, BOSTON_PLAT_MMCMDIV, &mmcmdiv);
+	if (err) {
+		pr_err("failed to read mmcm_div register: %d\n", err);
+		return;
+	}
+
+	in_freq = ext_field(mmcmdiv, BOSTON_PLAT_MMCMDIV_INPUT) * 1000000;
+	mul = ext_field(mmcmdiv, BOSTON_PLAT_MMCMDIV_MUL);
+
+	sys_div = ext_field(mmcmdiv, BOSTON_PLAT_MMCMDIV_CLK0DIV);
+	sys_freq = mult_frac(in_freq, mul, sys_div);
+
+	cpu_div = ext_field(mmcmdiv, BOSTON_PLAT_MMCMDIV_CLK1DIV);
+	cpu_freq = mult_frac(in_freq, mul, cpu_div);
+
+	state = kzalloc(sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return;
+
+	clk = clk_register_fixed_rate(NULL, "input", NULL, 0, in_freq);
+	if (IS_ERR(clk)) {
+		pr_err("failed to register input clock: %ld\n", PTR_ERR(clk));
+		return;
+	}
+	state->clks[BOSTON_CLK_INPUT] = clk;
+
+	clk = clk_register_fixed_rate(NULL, "sys", "input", 0, sys_freq);
+	if (IS_ERR(clk)) {
+		pr_err("failed to register sys clock: %ld\n", PTR_ERR(clk));
+		return;
+	}
+	state->clks[BOSTON_CLK_SYS] = clk;
+
+	clk = clk_register_fixed_rate(NULL, "cpu", "input", 0, cpu_freq);
+	if (IS_ERR(clk)) {
+		pr_err("failed to register cpu clock: %ld\n", PTR_ERR(clk));
+		return;
+	}
+	state->clks[BOSTON_CLK_CPU] = clk;
+
+	state->onecell_data.clks = state->clks;
+	state->onecell_data.clk_num = BOSTON_CLK_COUNT;
+
+	err = of_clk_add_provider(np, of_clk_src_onecell_get,
+				  &state->onecell_data);
+	if (err)
+		pr_err("failed to add DT provider: %d\n", err);
+}
+CLK_OF_DECLARE(clk_boston, "img,boston-clock", clk_boston_setup);
-- 
2.10.0

Re: [PATCH v3 17/18] clk: boston: Add a driver for MIPS Boston board clocks

[Paul Burton]

[-- Attachment #1.1: Type: text/plain, Size: 4652 bytes --]

On Wednesday, 5 October 2016 18:18:23 BST Paul Burton wrote:
> Add a driver for the clocks provided by the MIPS Boston board from
> Imagination Technologies. 2 clocks are provided - the system clock & the
> CPU clock - and each is a simple fixed rate clock whose frequency can be
> determined by reading a register provided by the board.
> 
> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@codeaurora.org>
> Cc: linux-clk@vger.kernel.org

Hello,

I realise the merge window may be a busy time for you, but would it be possible to get 
review of this (preferrably acks!) so Ralf can take it through the MIPS tree as part of Boston 
board support? It hasn't changed since v2 which was posted on 30th August.

Thanks,
    Paul

> 
> ---
> 
> Changes in v3: None
> Changes in v2:
> - Support BOSTON_CLK_INPUT.
> - Register clocks with clk_register_fixed_rate during boot, removing need
> for clk_ops. - s/uint32_t/u32/.
> - Move driver to a vendor directory.
> 
>  drivers/clk/Kconfig             |   1 +
>  drivers/clk/Makefile            |   1 +
>  drivers/clk/imgtec/Kconfig      |  10 ++++
>  drivers/clk/imgtec/Makefile     |   1 +
>  drivers/clk/imgtec/clk-boston.c | 101
> ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 114 insertions(+)
>  create mode 100644 drivers/clk/imgtec/Kconfig
>  create mode 100644 drivers/clk/imgtec/Makefile
>  create mode 100644 drivers/clk/imgtec/clk-boston.c
> 
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index e2d9bd7..a6c7e03 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -210,6 +210,7 @@ config COMMON_CLK_OXNAS
> 
>  source "drivers/clk/bcm/Kconfig"
>  source "drivers/clk/hisilicon/Kconfig"
> +source "drivers/clk/imgtec/Kconfig"
>  source "drivers/clk/meson/Kconfig"
>  source "drivers/clk/mvebu/Kconfig"
>  source "drivers/clk/qcom/Kconfig"
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 3b6f9cf..8c4c425 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -60,6 +60,7 @@ obj-y					
+= bcm/
>  obj-$(CONFIG_ARCH_BERLIN)		+= berlin/
>  obj-$(CONFIG_H8300)			+= h8300/
>  obj-$(CONFIG_ARCH_HISI)			+= hisilicon/
> +obj-y					+= imgtec/
>  obj-$(CONFIG_ARCH_MXC)			+= imx/
>  obj-$(CONFIG_MACH_INGENIC)		+= ingenic/
>  obj-$(CONFIG_COMMON_CLK_KEYSTONE)	+= keystone/
> diff --git a/drivers/clk/imgtec/Kconfig b/drivers/clk/imgtec/Kconfig
> new file mode 100644
> index 0000000..c2ea745
> --- /dev/null
> +++ b/drivers/clk/imgtec/Kconfig
> @@ -0,0 +1,10 @@
> +config COMMON_CLK_BOSTON
> +	bool "Clock driver for MIPS Boston boards"
> +	depends on MIPS || COMPILE_TEST
> +	depends on OF
> +	select MFD_SYSCON
> +	---help---
> +	  Enable this to support the system & CPU clocks on the MIPS Boston
> +	  development board from Imagination Technologies. These are simple
> +	  fixed rate clocks whose rate is determined by reading a platform
> +	  provided register.
> diff --git a/drivers/clk/imgtec/Makefile b/drivers/clk/imgtec/Makefile
> new file mode 100644
> index 0000000..ac779b8
> --- /dev/null
> +++ b/drivers/clk/imgtec/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_COMMON_CLK_BOSTON)		+= clk-boston.o
> diff --git a/drivers/clk/imgtec/clk-boston.c
> b/drivers/clk/imgtec/clk-boston.c new file mode 100644
> index 0000000..14795b8
> --- /dev/null
> +++ b/drivers/clk/imgtec/clk-boston.c
> @@ -0,0 +1,101 @@
> +/*
> + * Copyright (C) 2016 Imagination Technologies
> + * Author: Paul Burton <paul.burton@imgtec.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/mfd/syscon.h>
> +
> +#include <dt-bindings/clock/boston-clock.h>
> +
> +#define BOSTON_PLAT_MMCMDIV		0x30
> +# define BOSTON_PLAT_MMCMDIV_CLK0DIV	(0xff << 0)
> +# define BOSTON_PLAT_MMCMDIV_INPUT	(0xff << 8)
> +# define BOSTON_PLAT_MMCMDIV_MUL	(0xff << 16)
> +# define BOSTON_PLAT_MMCMDIV_CLK1DIV	(0xff << 24)
> +
> +#define BOSTON_CLK_COUNT 3
> +
> +struct clk_boston_state {
> +	struct clk *clks[BOSTON_CLK_COUNT];
> +	struct clk_onecell_data onecell_data;
> +};
> +
> +static u32 ext_field(u32 val, u32 mask)
> +{
> +	return (val & mask) >> (ffs(mask) - 1);
> +}
> +
> +static void __init clk_boston_setup(struct device_node *np)
> +{
> +	unsigned long in_freq, cpu_freq, sys_freq;

[-- Attachment #1.2: Type: text/html, Size: 35096 bytes --]

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3 16/18] dt-bindings: Document img,boston-clock binding

[Rob Herring]

On Wed, Oct 05, 2016 at 06:18:22PM +0100, Paul Burton wrote:
> Add device tree binding documentation for the clocks provided by the
> MIPS Boston development board from Imagination Technologies, and a
> header file describing the available clocks for use by device trees &
> driver.
> 
> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@codeaurora.org>
> Cc: linux-clk@vger.kernel.org
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: devicetree@vger.kernel.org
> 
> ---
> 
> Changes in v3: None
> Changes in v2:
> - Add BOSTON_CLK_INPUT to expose the input clock.
> 
>  .../devicetree/bindings/clock/img,boston-clock.txt | 27 ++++++++++++++++++++++
>  include/dt-bindings/clock/boston-clock.h           | 14 +++++++++++
>  2 files changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/img,boston-clock.txt
>  create mode 100644 include/dt-bindings/clock/boston-clock.h
> 
> diff --git a/Documentation/devicetree/bindings/clock/img,boston-clock.txt b/Documentation/devicetree/bindings/clock/img,boston-clock.txt
> new file mode 100644
> index 0000000..c01ea60
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/img,boston-clock.txt
> @@ -0,0 +1,27 @@
> +Binding for Imagination Technologies MIPS Boston clock sources.
> +
> +This binding uses the common clock binding[1].
> +
> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> +
> +Required properties:
> +- compatible : Should be "img,boston-clock".
> +- #clock-cells : Should be set to 1.
> +  Values available for clock consumers can be found in the header file:
> +    <dt-bindings/clock/boston-clock.h>
> +- regmap : Phandle to the Boston platform register system controller.
> +  This should contain a phandle to the system controller node covering the
> +  platform registers provided by the Boston board.

Can you just make the clock node a child of the system controller and 
drop this?

Rob

Re: [PATCH v3 16/18] dt-bindings: Document img,boston-clock binding

[Paul Burton]

[-- Attachment #1.1: Type: text/plain, Size: 2410 bytes --]

On Monday, 10 October 2016 08:01:21 BST Rob Herring wrote:
> On Wed, Oct 05, 2016 at 06:18:22PM +0100, Paul Burton wrote:
> > Add device tree binding documentation for the clocks provided by the
> > MIPS Boston development board from Imagination Technologies, and a
> > header file describing the available clocks for use by device trees &
> > driver.
> > 
> > Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> > Cc: Michael Turquette <mturquette@baylibre.com>
> > Cc: Stephen Boyd <sboyd@codeaurora.org>
> > Cc: linux-clk@vger.kernel.org
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: devicetree@vger.kernel.org
> > 
> > ---
> > 
> > Changes in v3: None
> > Changes in v2:
> > - Add BOSTON_CLK_INPUT to expose the input clock.
> > 
> >  .../devicetree/bindings/clock/img,boston-clock.txt | 27
> >  ++++++++++++++++++++++ include/dt-bindings/clock/boston-clock.h         
> >   | 14 +++++++++++ 2 files changed, 41 insertions(+)
> >  create mode 100644
> >  Documentation/devicetree/bindings/clock/img,boston-clock.txt create mode
> >  100644 include/dt-bindings/clock/boston-clock.h
> > 
> > diff --git a/Documentation/devicetree/bindings/clock/img,boston-clock.txt
> > b/Documentation/devicetree/bindings/clock/img,boston-clock.txt new file
> > mode 100644
> > index 0000000..c01ea60
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/img,boston-clock.txt
> > @@ -0,0 +1,27 @@
> > +Binding for Imagination Technologies MIPS Boston clock sources.
> > +
> > +This binding uses the common clock binding[1].
> > +
> > +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> > +
> > +Required properties:
> > +- compatible : Should be "img,boston-clock".
> > +- #clock-cells : Should be set to 1.
> > +  Values available for clock consumers can be found in the header file:
> > +    <dt-bindings/clock/boston-clock.h>
> > +- regmap : Phandle to the Boston platform register system controller.
> > +  This should contain a phandle to the system controller node covering
> > the
> > +  platform registers provided by the Boston board.
> 
> Can you just make the clock node a child of the system controller and
> drop this?
> 
> Rob

Hi Rob,

As I mentioned before technically that could be done, but it would really not be at all 
reflective of the hardware & so seems somewhat contrary to the purpose of a device tree.

Thanks,
    Paul

[-- Attachment #1.2: Type: text/html, Size: 11783 bytes --]

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3 16/18] dt-bindings: Document img,boston-clock binding

[Paul Burton]

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

On Monday, 10 October 2016 08:01:21 BST Rob Herring wrote:
> On Wed, Oct 05, 2016 at 06:18:22PM +0100, Paul Burton wrote:
> > Add device tree binding documentation for the clocks provided by the
> > MIPS Boston development board from Imagination Technologies, and a
> > header file describing the available clocks for use by device trees &
> > driver.
> > 
> > Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> > Cc: Michael Turquette <mturquette@baylibre.com>
> > Cc: Stephen Boyd <sboyd@codeaurora.org>
> > Cc: linux-clk@vger.kernel.org
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: devicetree@vger.kernel.org
> > 
> > ---
> > 
> > Changes in v3: None
> > Changes in v2:
> > - Add BOSTON_CLK_INPUT to expose the input clock.
> > 
> >  .../devicetree/bindings/clock/img,boston-clock.txt | 27
> >  ++++++++++++++++++++++ include/dt-bindings/clock/boston-clock.h         
> >   | 14 +++++++++++ 2 files changed, 41 insertions(+)
> >  create mode 100644
> >  Documentation/devicetree/bindings/clock/img,boston-clock.txt create mode
> >  100644 include/dt-bindings/clock/boston-clock.h
> > 
> > diff --git a/Documentation/devicetree/bindings/clock/img,boston-clock.txt
> > b/Documentation/devicetree/bindings/clock/img,boston-clock.txt new file
> > mode 100644
> > index 0000000..c01ea60
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/img,boston-clock.txt
> > @@ -0,0 +1,27 @@
> > +Binding for Imagination Technologies MIPS Boston clock sources.
> > +
> > +This binding uses the common clock binding[1].
> > +
> > +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> > +
> > +Required properties:
> > +- compatible : Should be "img,boston-clock".
> > +- #clock-cells : Should be set to 1.
> > +  Values available for clock consumers can be found in the header file:
> > +    <dt-bindings/clock/boston-clock.h>
> > +- regmap : Phandle to the Boston platform register system controller.
> > +  This should contain a phandle to the system controller node covering
> > the
> > +  platform registers provided by the Boston board.
> 
> Can you just make the clock node a child of the system controller and
> drop this?
> 
> Rob

Hi Rob,
 
(Apologies to anyone who received my last; my mail client seems to be 
misconfigured & previously sent HTML mail.)

As I mentioned before technically that could be done, but it would really not 
be at all reflective of the hardware & so seems somewhat contrary to the 
purpose of a device tree.
 
Thanks,
    Paul

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3 16/18] dt-bindings: Document img,boston-clock binding

[Rob Herring]

On Tue, Oct 11, 2016 at 11:00 AM, Paul Burton <paul.burton@imgtec.com> wrote:
> On Monday, 10 October 2016 08:01:21 BST Rob Herring wrote:
>> On Wed, Oct 05, 2016 at 06:18:22PM +0100, Paul Burton wrote:
>> > Add device tree binding documentation for the clocks provided by the
>> > MIPS Boston development board from Imagination Technologies, and a
>> > header file describing the available clocks for use by device trees &
>> > driver.
>> >
>> > Signed-off-by: Paul Burton <paul.burton@imgtec.com>
>> > Cc: Michael Turquette <mturquette@baylibre.com>
>> > Cc: Stephen Boyd <sboyd@codeaurora.org>
>> > Cc: linux-clk@vger.kernel.org
>> > Cc: Rob Herring <robh+dt@kernel.org>
>> > Cc: Mark Rutland <mark.rutland@arm.com>
>> > Cc: devicetree@vger.kernel.org
>> >
>> > ---
>> >
>> > Changes in v3: None
>> > Changes in v2:
>> > - Add BOSTON_CLK_INPUT to expose the input clock.
>> >
>> >  .../devicetree/bindings/clock/img,boston-clock.txt | 27
>> >  ++++++++++++++++++++++ include/dt-bindings/clock/boston-clock.h
>> >   | 14 +++++++++++ 2 files changed, 41 insertions(+)
>> >  create mode 100644
>> >  Documentation/devicetree/bindings/clock/img,boston-clock.txt create mode
>> >  100644 include/dt-bindings/clock/boston-clock.h
>> >
>> > diff --git a/Documentation/devicetree/bindings/clock/img,boston-clock.txt
>> > b/Documentation/devicetree/bindings/clock/img,boston-clock.txt new file
>> > mode 100644
>> > index 0000000..c01ea60
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/clock/img,boston-clock.txt
>> > @@ -0,0 +1,27 @@
>> > +Binding for Imagination Technologies MIPS Boston clock sources.
>> > +
>> > +This binding uses the common clock binding[1].
>> > +
>> > +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>> > +
>> > +Required properties:
>> > +- compatible : Should be "img,boston-clock".
>> > +- #clock-cells : Should be set to 1.
>> > +  Values available for clock consumers can be found in the header file:
>> > +    <dt-bindings/clock/boston-clock.h>
>> > +- regmap : Phandle to the Boston platform register system controller.
>> > +  This should contain a phandle to the system controller node covering
>> > the
>> > +  platform registers provided by the Boston board.
>>
>> Can you just make the clock node a child of the system controller and
>> drop this?
>>
>> Rob
>
> Hi Rob,
>
> (Apologies to anyone who received my last; my mail client seems to be
> misconfigured & previously sent HTML mail.)
>
> As I mentioned before technically that could be done, but it would really not
> be at all reflective of the hardware & so seems somewhat contrary to the
> purpose of a device tree.

Given that you need a reference back to the system controller, it does
match the h/w. The system controller h/w contains various functions,
therefore the system controller node should contain nodes for those
functions (or the sys ctrlr itself could be the clock provider node
with no child nodes). Otherwise, what is the parent of the clock node?
Root? Root should generally be the top level devices of the SoC,
though it gets used for things which have no good parent.

Rob

Re: [PATCH v3 16/18] dt-bindings: Document img,boston-clock binding

[Paul Burton]

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

On Tuesday, 11 October 2016 15:06:46 BST Rob Herring wrote:
> On Tue, Oct 11, 2016 at 11:00 AM, Paul Burton <paul.burton@imgtec.com> 
wrote:
> > On Monday, 10 October 2016 08:01:21 BST Rob Herring wrote:
> >> On Wed, Oct 05, 2016 at 06:18:22PM +0100, Paul Burton wrote:
> >> > Add device tree binding documentation for the clocks provided by the
> >> > MIPS Boston development board from Imagination Technologies, and a
> >> > header file describing the available clocks for use by device trees &
> >> > driver.
> >> > 
> >> > Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> >> > Cc: Michael Turquette <mturquette@baylibre.com>
> >> > Cc: Stephen Boyd <sboyd@codeaurora.org>
> >> > Cc: linux-clk@vger.kernel.org
> >> > Cc: Rob Herring <robh+dt@kernel.org>
> >> > Cc: Mark Rutland <mark.rutland@arm.com>
> >> > Cc: devicetree@vger.kernel.org
> >> > 
> >> > ---
> >> > 
> >> > Changes in v3: None
> >> > Changes in v2:
> >> > - Add BOSTON_CLK_INPUT to expose the input clock.
> >> > 
> >> >  .../devicetree/bindings/clock/img,boston-clock.txt | 27
> >> >  ++++++++++++++++++++++ include/dt-bindings/clock/boston-clock.h
> >> >  
> >> >   | 14 +++++++++++ 2 files changed, 41 insertions(+)
> >> >  
> >> >  create mode 100644
> >> >  Documentation/devicetree/bindings/clock/img,boston-clock.txt create
> >> >  mode
> >> >  100644 include/dt-bindings/clock/boston-clock.h
> >> > 
> >> > diff --git
> >> > a/Documentation/devicetree/bindings/clock/img,boston-clock.txt
> >> > b/Documentation/devicetree/bindings/clock/img,boston-clock.txt new file
> >> > mode 100644
> >> > index 0000000..c01ea60
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/clock/img,boston-clock.txt
> >> > @@ -0,0 +1,27 @@
> >> > +Binding for Imagination Technologies MIPS Boston clock sources.
> >> > +
> >> > +This binding uses the common clock binding[1].
> >> > +
> >> > +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> >> > +
> >> > +Required properties:
> >> > +- compatible : Should be "img,boston-clock".
> >> > +- #clock-cells : Should be set to 1.
> >> > +  Values available for clock consumers can be found in the header
> >> > file:
> >> > +    <dt-bindings/clock/boston-clock.h>
> >> > +- regmap : Phandle to the Boston platform register system controller.
> >> > +  This should contain a phandle to the system controller node covering
> >> > the
> >> > +  platform registers provided by the Boston board.
> >> 
> >> Can you just make the clock node a child of the system controller and
> >> drop this?
> >> 
> >> Rob
> > 
> > Hi Rob,
> > 
> > (Apologies to anyone who received my last; my mail client seems to be
> > misconfigured & previously sent HTML mail.)
> > 
> > As I mentioned before technically that could be done, but it would really
> > not be at all reflective of the hardware & so seems somewhat contrary to
> > the purpose of a device tree.
> 
> Given that you need a reference back to the system controller, it does
> match the h/w. The system controller h/w contains various functions,
> therefore the system controller node should contain nodes for those
> functions (or the sys ctrlr itself could be the clock provider node
> with no child nodes). Otherwise, what is the parent of the clock node?
> Root? Root should generally be the top level devices of the SoC,
> though it gets used for things which have no good parent.
> 
> Rob

Hi Rob,

The "system controller" here is a bunch of registers which contain information 
about the system - nothing more & nothing less. There are a few random bits of 
functionality such as system level reset exposed through them, but things like 
clocks are not part of some coherent block of hardware known as the system 
controller. The register exposing information about the clocks has no actual 
connection to the clocks at all - it's just a dumb register whose value is 
filled in by whomever generates the FPGA bitfile. I don't see how that can be 
reasonably seen as the clocks being a child of this ecclectic bunch of 
registers.

Perhaps the use of syscon has been misleading here? I'm using the syscon code 
purely as a nice way to obtain a regmap to that bunch of registers. Please 
believe me when I say I know this hardware well enough to know that there 
isn't a coherent block of system controller hardware that provides the clocks 
here.

Thanks,
    Paul

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3 16/18] dt-bindings: Document img,boston-clock binding

[Stephen Boyd]

On 10/11, Paul Burton wrote:
> On Tuesday, 11 October 2016 15:06:46 BST Rob Herring wrote:
> > On Tue, Oct 11, 2016 at 11:00 AM, Paul Burton <paul.burton@imgtec.com> 
> wrote:
> > > On Monday, 10 October 2016 08:01:21 BST Rob Herring wrote:
> > >> On Wed, Oct 05, 2016 at 06:18:22PM +0100, Paul Burton wrote:
> > >> > Add device tree binding documentation for the clocks provided by the
> > >> > MIPS Boston development board from Imagination Technologies, and a
> > >> > header file describing the available clocks for use by device trees &
> > >> > driver.
> > >> > 
> > >> > Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> > >> > Cc: Michael Turquette <mturquette@baylibre.com>
> > >> > Cc: Stephen Boyd <sboyd@codeaurora.org>
> > >> > Cc: linux-clk@vger.kernel.org
> > >> > Cc: Rob Herring <robh+dt@kernel.org>
> > >> > Cc: Mark Rutland <mark.rutland@arm.com>
> > >> > Cc: devicetree@vger.kernel.org
> > >> > 
> > >> > ---
> > >> > 
> > >> > Changes in v3: None
> > >> > Changes in v2:
> > >> > - Add BOSTON_CLK_INPUT to expose the input clock.
> > >> > 
> > >> >  .../devicetree/bindings/clock/img,boston-clock.txt | 27
> > >> >  ++++++++++++++++++++++ include/dt-bindings/clock/boston-clock.h
> > >> >  
> > >> >   | 14 +++++++++++ 2 files changed, 41 insertions(+)
> > >> >  
> > >> >  create mode 100644
> > >> >  Documentation/devicetree/bindings/clock/img,boston-clock.txt create
> > >> >  mode
> > >> >  100644 include/dt-bindings/clock/boston-clock.h
> > >> > 
> > >> > diff --git
> > >> > a/Documentation/devicetree/bindings/clock/img,boston-clock.txt
> > >> > b/Documentation/devicetree/bindings/clock/img,boston-clock.txt new file
> > >> > mode 100644
> > >> > index 0000000..c01ea60
> > >> > --- /dev/null
> > >> > +++ b/Documentation/devicetree/bindings/clock/img,boston-clock.txt
> > >> > @@ -0,0 +1,27 @@
> > >> > +Binding for Imagination Technologies MIPS Boston clock sources.
> > >> > +
> > >> > +This binding uses the common clock binding[1].
> > >> > +
> > >> > +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> > >> > +
> > >> > +Required properties:
> > >> > +- compatible : Should be "img,boston-clock".
> > >> > +- #clock-cells : Should be set to 1.
> > >> > +  Values available for clock consumers can be found in the header
> > >> > file:
> > >> > +    <dt-bindings/clock/boston-clock.h>
> > >> > +- regmap : Phandle to the Boston platform register system controller.
> > >> > +  This should contain a phandle to the system controller node covering
> > >> > the
> > >> > +  platform registers provided by the Boston board.
> > >> 
> > >> Can you just make the clock node a child of the system controller and
> > >> drop this?
> > >> 
> > >> Rob
> > > 
> > > Hi Rob,
> > > 
> > > (Apologies to anyone who received my last; my mail client seems to be
> > > misconfigured & previously sent HTML mail.)
> > > 
> > > As I mentioned before technically that could be done, but it would really
> > > not be at all reflective of the hardware & so seems somewhat contrary to
> > > the purpose of a device tree.
> > 
> > Given that you need a reference back to the system controller, it does
> > match the h/w. The system controller h/w contains various functions,
> > therefore the system controller node should contain nodes for those
> > functions (or the sys ctrlr itself could be the clock provider node
> > with no child nodes). Otherwise, what is the parent of the clock node?
> > Root? Root should generally be the top level devices of the SoC,
> > though it gets used for things which have no good parent.
> > 
> > Rob
> 
> Hi Rob,
> 
> The "system controller" here is a bunch of registers which contain information 
> about the system - nothing more & nothing less. There are a few random bits of 
> functionality such as system level reset exposed through them, but things like 
> clocks are not part of some coherent block of hardware known as the system 
> controller. The register exposing information about the clocks has no actual 
> connection to the clocks at all - it's just a dumb register whose value is 
> filled in by whomever generates the FPGA bitfile. 

This sounds pretty much how every clk controller or syscon/mfd is
made. A dumb register set that controls a bunch of hard macros
placed by hardware designers. It just so happens that there are
other things in this register set that aren't clk controls.

> I don't see how that can be 
> reasonably seen as the clocks being a child of this ecclectic bunch of 
> registers.

Can you further describe what being a child device means to you?
There must be some reason why a child device is wrong from your
perspective that isn't coming across here.

> 
> Perhaps the use of syscon has been misleading here? I'm using the syscon code 
> purely as a nice way to obtain a regmap to that bunch of registers. Please 
> believe me when I say I know this hardware well enough to know that there 
> isn't a coherent block of system controller hardware that provides the clocks 
> here.
> 

The problem I have, from a DT perspective, is that the clks are
not on the board. Clks that live on the board go to the root of
the DT tree under / or some container clks node. In this case the
clks are in the SoC, so they should go into the SoC node, not the
root. But the SoC node should have a reg property for each child
node, and this node wouldn't have a reg property because it uses
a regmap, so from a DT perspective that's also the wrong place to
put this node.

Therefore, the best solution is to make the clk controller node a
child of the syscon, and then the reg property doesn't exist in
the clk controller node. This also nicely removes any linuxism of
needing to have a regmap property (regmap is not really a
hardware concept and is fairly linux specific) because we can
grab the regmap handle from the parent device/node.

Technically having the child node for the clk controller part is
abusing DT. We should really only have one node for the syscon,
which probes a driver that then creates platform devices in
software to match up with other drivers for the specific
functions that are accessible through the dumb register set. But
since we have MFD/syscons quite often, and things get sort of
messy when all these different things are going on within one
node and thus one big driver, we allow MFDs to be described in DT
with sub-nodes that are for the sub-functions so that we can have
drivers match up appropriately. Otherwise we're left with a big
driver for the MFD that becomes sort of like a board file to
register sub devices, or a mash up of all these different
functions that use one struct device.

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