[PATCH v2 0/2] CREG clk driver for NXP LPC18xx family

[PATCH v2 0/2] CREG clk driver for NXP LPC18xx family

[Joachim Eastwood]

This patch set adds a clk driver for the low power clocks found in
the CREG block on lpc18xx. CREG is a collection of miscellaneous
configuration registers that can be accessed through a syscon
regmap interface. The clk driver makes it possible to setup and
enabled these two clocks.

This need to support peripherals like the internal RTC on the
lpc18xx platform.

Mike/Stephen is this what you envisioned?
It seems to work, but I currently don't have a setup that requires
the 32khz clock early so that part isn't really tested.

The probe deferral seems to work for the RTC:
[    3.386861] lpc24xx-rtc 40046000.rtc: error getting rtc clock
[    6.737615] lpc24xx-rtc 40046000.rtc: rtc core: registered lpc24xx-rtc as rtc0


Changes since v1:
 - Split driver with one early init and one platform driver
   probe.

Based on v4.5-rc1.

Joachim Eastwood (2):
  clk: add lpc18xx creg clk driver
  doc: dt: add documentation for lpc1850-creg-clk driver

 .../devicetree/bindings/clock/lpc1850-creg-clk.txt |  52 ++++++
 drivers/clk/nxp/Makefile                           |   1 +
 drivers/clk/nxp/clk-lpc18xx-creg.c                 | 205 +++++++++++++++++++++
 3 files changed, 258 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/lpc1850-creg-clk.txt
 create mode 100644 drivers/clk/nxp/clk-lpc18xx-creg.c

-- 
1.8.0

[PATCH v2 1/2] clk: add lpc18xx creg clk driver

[Joachim Eastwood]

The CREG block on lpc18xx contains configuration register
for two low power clocks. Support enabling of these two
clocks with a clk driver that access CREG trough the
syscon regmap interface.

These clocks are needed to support peripherals like the
internal RTC on lpc18xx.

Signed-off-by: Joachim Eastwood <manabian@gmail.com>
---
 drivers/clk/nxp/Makefile           |   1 +
 drivers/clk/nxp/clk-lpc18xx-creg.c | 205 +++++++++++++++++++++++++++++++++++++
 2 files changed, 206 insertions(+)
 create mode 100644 drivers/clk/nxp/clk-lpc18xx-creg.c

diff --git a/drivers/clk/nxp/Makefile b/drivers/clk/nxp/Makefile
index 607bd48c6563..d456ee6cc3d3 100644
--- a/drivers/clk/nxp/Makefile
+++ b/drivers/clk/nxp/Makefile
@@ -1,3 +1,4 @@
 obj-$(CONFIG_ARCH_LPC18XX)	+= clk-lpc18xx-cgu.o
 obj-$(CONFIG_ARCH_LPC18XX)	+= clk-lpc18xx-ccu.o
+obj-$(CONFIG_ARCH_LPC18XX)	+= clk-lpc18xx-creg.o
 obj-$(CONFIG_ARCH_LPC32XX)	+= clk-lpc32xx.o
diff --git a/drivers/clk/nxp/clk-lpc18xx-creg.c b/drivers/clk/nxp/clk-lpc18xx-creg.c
new file mode 100644
index 000000000000..af2b2ae3aea7
--- /dev/null
+++ b/drivers/clk/nxp/clk-lpc18xx-creg.c
@@ -0,0 +1,205 @@
+/*
+ * Clk driver for NXP LPC18xx/43xx Configuration Registers (CREG)
+ *
+ * Copyright (C) 2015 Joachim Eastwood <manabian@gmail.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/delay.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define LPC18XX_CREG_CREG0			0x004
+#define  LPC18XX_CREG_CREG0_EN1KHZ		BIT(0)
+#define  LPC18XX_CREG_CREG0_EN32KHZ		BIT(1)
+#define  LPC18XX_CREG_CREG0_RESET32KHZ		BIT(2)
+#define  LPC18XX_CREG_CREG0_PD32KHZ		BIT(3)
+
+#define to_clk_creg(_hw) container_of(_hw, struct clk_creg_data, hw)
+
+enum {
+	CREG_CLK_1KHZ,
+	CREG_CLK_32KHZ,
+	CREG_CLK_MAX,
+};
+
+struct clk_creg_data {
+	struct clk_hw hw;
+	const char *name;
+	struct regmap *reg;
+	unsigned int en_mask;
+	const struct clk_ops *ops;
+};
+
+#define CREG_CLK(_name, _emask, _ops)		\
+{						\
+	.name = _name,				\
+	.en_mask = LPC18XX_CREG_CREG0_##_emask,	\
+	.ops = &_ops,				\
+}
+
+static int clk_creg_32k_prepare(struct clk_hw *hw)
+{
+	struct clk_creg_data *creg = to_clk_creg(hw);
+	int ret;
+
+	ret = regmap_update_bits(creg->reg, LPC18XX_CREG_CREG0,
+				 LPC18XX_CREG_CREG0_PD32KHZ |
+				 LPC18XX_CREG_CREG0_RESET32KHZ, 0);
+
+	/*
+	 * Powering up the 32k oscillator takes a long while
+	 * and sadly there aren't any status bit to poll.
+	 */
+	msleep(2500);
+
+	return ret;
+}
+
+static void clk_creg_32k_unprepare(struct clk_hw *hw)
+{
+	struct clk_creg_data *creg = to_clk_creg(hw);
+
+	regmap_update_bits(creg->reg, LPC18XX_CREG_CREG0,
+			   LPC18XX_CREG_CREG0_PD32KHZ,
+			   LPC18XX_CREG_CREG0_PD32KHZ);
+}
+
+static int clk_creg_32k_is_prepared(struct clk_hw *hw)
+{
+	struct clk_creg_data *creg = to_clk_creg(hw);
+	u32 reg;
+
+	regmap_read(creg->reg, LPC18XX_CREG_CREG0, &reg);
+
+	return !(reg & LPC18XX_CREG_CREG0_PD32KHZ) &&
+	       !(reg & LPC18XX_CREG_CREG0_RESET32KHZ);
+}
+
+static unsigned long clk_creg_1k_recalc_rate(struct clk_hw *hw,
+					     unsigned long parent_rate)
+{
+	return parent_rate / 32;
+}
+
+static int clk_creg_enable(struct clk_hw *hw)
+{
+	struct clk_creg_data *creg = to_clk_creg(hw);
+
+	return regmap_update_bits(creg->reg, LPC18XX_CREG_CREG0,
+				  creg->en_mask, creg->en_mask);
+}
+
+static void clk_creg_disable(struct clk_hw *hw)
+{
+	struct clk_creg_data *creg = to_clk_creg(hw);
+
+	regmap_update_bits(creg->reg, LPC18XX_CREG_CREG0,
+			   creg->en_mask, 0);
+}
+
+static const struct clk_ops clk_creg_32k = {
+	.enable		= clk_creg_enable,
+	.disable	= clk_creg_disable,
+	.prepare	= clk_creg_32k_prepare,
+	.unprepare	= clk_creg_32k_unprepare,
+	.is_prepared	= clk_creg_32k_is_prepared,
+};
+
+static const struct clk_ops clk_creg_1k = {
+	.enable		= clk_creg_enable,
+	.disable	= clk_creg_disable,
+	.recalc_rate	= clk_creg_1k_recalc_rate,
+};
+
+static struct clk_creg_data clk_creg_clocks[] = {
+	[CREG_CLK_1KHZ]  = CREG_CLK("1khz_clk",  EN1KHZ,  clk_creg_1k),
+	[CREG_CLK_32KHZ] = CREG_CLK("32khz_clk", EN32KHZ, clk_creg_32k),
+};
+
+static struct clk *clk_register_creg_clk(struct clk_creg_data *creg_clk,
+					 const char **parent_name,
+					 struct regmap *syscon)
+{
+	struct clk_init_data init;
+
+	init.ops = creg_clk->ops;
+	init.name = creg_clk->name;
+	init.parent_names = parent_name;
+	init.num_parents = 1;
+
+	creg_clk->reg = syscon;
+	creg_clk->hw.init = &init;
+
+	return clk_register(NULL, &creg_clk->hw);
+}
+
+static struct clk *clk_creg[CREG_CLK_MAX];
+static struct clk_onecell_data clk_base_data = {
+	.clks = clk_creg,
+	.clk_num = CREG_CLK_MAX,
+};
+
+static void __init lpc18xx_creg_clk_init(struct device_node *np)
+{
+	const char *clk_32khz_parent;
+	struct regmap *syscon;
+
+	syscon = syscon_node_to_regmap(np->parent);
+	if (IS_ERR(syscon)) {
+		pr_err("%s: syscon lookup failed\n", __func__);
+		return;
+	}
+
+	clk_32khz_parent = of_clk_get_parent_name(np, 0);
+
+	clk_creg[CREG_CLK_32KHZ] =
+		clk_register_creg_clk(&clk_creg_clocks[CREG_CLK_32KHZ],
+				      &clk_32khz_parent, syscon);
+	clk_creg[CREG_CLK_1KHZ] = ERR_PTR(-EPROBE_DEFER);
+
+	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_base_data);
+}
+CLK_OF_DECLARE(lpc18xx_creg_clk, "nxp,lpc1850-creg-clk", lpc18xx_creg_clk_init);
+
+static int lpc18xx_creg_clk_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct regmap *syscon;
+
+	syscon = syscon_node_to_regmap(np->parent);
+	if (IS_ERR(syscon)) {
+		dev_err(&pdev->dev, "syscon lookup failed\n");
+		return PTR_ERR(syscon);
+	}
+
+	clk_creg[CREG_CLK_1KHZ] =
+		clk_register_creg_clk(&clk_creg_clocks[CREG_CLK_1KHZ],
+				      &clk_creg_clocks[CREG_CLK_32KHZ].name,
+				      syscon);
+
+	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_base_data);
+
+	return 0;
+}
+
+static const struct of_device_id lpc18xx_creg_clk_of_match[] = {
+	{ .compatible = "nxp,lpc1850-creg-clk", },
+	{},
+};
+
+static struct platform_driver lpc18xx_creg_clk_driver = {
+	.probe = lpc18xx_creg_clk_probe,
+	.driver = {
+		.name = "lpc18xx-creg-clk",
+		.of_match_table = lpc18xx_creg_clk_of_match,
+	},
+};
+builtin_platform_driver(lpc18xx_creg_clk_driver);
-- 
1.8.0

[PATCH v2 2/2] doc: dt: add documentation for lpc1850-creg-clk driver

[Joachim Eastwood]

Add DT binding documentation for lpc1850-creg-clk driver.

Signed-off-by: Joachim Eastwood <manabian@gmail.com>
---
 .../devicetree/bindings/clock/lpc1850-creg-clk.txt | 52 ++++++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/lpc1850-creg-clk.txt

diff --git a/Documentation/devicetree/bindings/clock/lpc1850-creg-clk.txt b/Documentation/devicetree/bindings/clock/lpc1850-creg-clk.txt
new file mode 100644
index 000000000000..0c83d373b766
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/lpc1850-creg-clk.txt
@@ -0,0 +1,52 @@
+* NXP LPC1850 CREG clocks
+
+The NXP LPC18xx/43xx CREG (Configuration Registers) block contains
+control registers for two low speed clocks. One of the clocks is a
+32 kHz oscillator driver with power up/down and clock gating. Next
+is a fixed divider that creates a 1 kHz clock from the 32 kHz osc.
+
+These clocks are used by the RTC and the Event Router peripherials.
+The 32 kHz can also be routed to other peripherials to enable low
+power modes.
+
+This binding uses the common clock binding:
+    Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible:
+	Should be "nxp,lpc1850-creg-clk"
+- #clock-cells:
+	Shall have value <1>.
+- clocks:
+	Shall contain a phandle to the fix 32 kHz crystall.
+
+The creg-clk node must be a child of the creg syscon node.
+
+The following clocks are available from the clock node.
+
+Clock ID	Name
+   0		 1 kHz clock
+   1		32 kHz Oscillator
+
+Example:
+soc {
+	creg: syscon@40043000 {
+		compatible = "nxp,lpc1850-creg", "syscon", "simple-mfd";
+		reg = <0x40043000 0x1000>;
+
+		creg_clk: clock-controller@004 {
+			compatible = "nxp,lpc1850-creg-clk";
+			clocks = <&xtal32>;
+			#clock-cells = <1>;
+		};
+
+		...
+	};
+
+	rtc: rtc@40046000 {
+		...
+		clocks = <&creg_clk 0>, <&ccu1 CLK_CPU_BUS>;
+		clock-names = "rtc", "reg";
+		...
+	};
+};
-- 
1.8.0

Re: [PATCH v2 1/2] clk: add lpc18xx creg clk driver

[Stephen Boyd]

On 02/20, Joachim Eastwood wrote:
> +static int lpc18xx_creg_clk_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct regmap *syscon;
> +
> +	syscon = syscon_node_to_regmap(np->parent);
> +	if (IS_ERR(syscon)) {
> +		dev_err(&pdev->dev, "syscon lookup failed\n");
> +		return PTR_ERR(syscon);
> +	}
> +
> +	clk_creg[CREG_CLK_1KHZ] =
> +		clk_register_creg_clk(&clk_creg_clocks[CREG_CLK_1KHZ],
> +				      &clk_creg_clocks[CREG_CLK_32KHZ].name,
> +				      syscon);
> +
> +	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_base_data);

If we use the same clk_base_data both times then why can't we
just insert the 1KHz clk into the clk_creg array when the
platform device probes? That would be racy if something was
trying to read the array when it's being modified, so the
alternative non-racy solution would be to make two clk_base_data
structures, one in CLK_OF_DECLARE() and one in platform device
probe.

Otherwise this looks like what I was envisioning.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] clk: add lpc18xx creg clk driver

[Stephen Boyd]

On 02/20, Joachim Eastwood wrote:
> +static int lpc18xx_creg_clk_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct regmap *syscon;
> +
> +	syscon = syscon_node_to_regmap(np->parent);
> +	if (IS_ERR(syscon)) {
> +		dev_err(&pdev->dev, "syscon lookup failed\n");
> +		return PTR_ERR(syscon);
> +	}
> +
> +	clk_creg[CREG_CLK_1KHZ] =
> +		clk_register_creg_clk(&clk_creg_clocks[CREG_CLK_1KHZ],
> +				      &clk_creg_clocks[CREG_CLK_32KHZ].name,
> +				      syscon);
> +
> +	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_base_data);

If we use the same clk_base_data both times then why can't we
just insert the 1KHz clk into the clk_creg array when the
platform device probes? That would be racy if something was
trying to read the array when it's being modified, so the
alternative non-racy solution would be to make two clk_base_data
structures, one in CLK_OF_DECLARE() and one in platform device
probe.

Otherwise this looks like what I was envisioning.

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

Re: [PATCH v2 2/2] doc: dt: add documentation for lpc1850-creg-clk driver

[Rob Herring]

On Sat, Feb 20, 2016 at 07:53:37PM +0100, Joachim Eastwood wrote:
> Add DT binding documentation for lpc1850-creg-clk driver.
> 
> Signed-off-by: Joachim Eastwood <manabian@gmail.com>
> ---
>  .../devicetree/bindings/clock/lpc1850-creg-clk.txt | 52 ++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/lpc1850-creg-clk.txt
> 
> diff --git a/Documentation/devicetree/bindings/clock/lpc1850-creg-clk.txt b/Documentation/devicetree/bindings/clock/lpc1850-creg-clk.txt
> new file mode 100644
> index 000000000000..0c83d373b766
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/lpc1850-creg-clk.txt
> @@ -0,0 +1,52 @@
> +* NXP LPC1850 CREG clocks
> +
> +The NXP LPC18xx/43xx CREG (Configuration Registers) block contains
> +control registers for two low speed clocks. One of the clocks is a
> +32 kHz oscillator driver with power up/down and clock gating. Next
> +is a fixed divider that creates a 1 kHz clock from the 32 kHz osc.
> +
> +These clocks are used by the RTC and the Event Router peripherials.
> +The 32 kHz can also be routed to other peripherials to enable low
> +power modes.
> +
> +This binding uses the common clock binding:
> +    Documentation/devicetree/bindings/clock/clock-bindings.txt
> +
> +Required properties:
> +- compatible:
> +	Should be "nxp,lpc1850-creg-clk"
> +- #clock-cells:
> +	Shall have value <1>.
> +- clocks:
> +	Shall contain a phandle to the fix 32 kHz crystall.
> +
> +The creg-clk node must be a child of the creg syscon node.
> +
> +The following clocks are available from the clock node.
> +
> +Clock ID	Name
> +   0		 1 kHz clock
> +   1		32 kHz Oscillator
> +
> +Example:
> +soc {
> +	creg: syscon@40043000 {
> +		compatible = "nxp,lpc1850-creg", "syscon", "simple-mfd";
> +		reg = <0x40043000 0x1000>;
> +
> +		creg_clk: clock-controller@004 {

What is 4? A unit address should have a corresponding reg prop. and no 
leading zeros.

> +			compatible = "nxp,lpc1850-creg-clk";
> +			clocks = <&xtal32>;
> +			#clock-cells = <1>;
> +		};

Re: [PATCH v2 2/2] doc: dt: add documentation for lpc1850-creg-clk driver

[Joachim Eastwood]

On 23 February 2016 at 19:08, Rob Herring <robh@kernel.org> wrote:
> On Sat, Feb 20, 2016 at 07:53:37PM +0100, Joachim Eastwood wrote:
>> Add DT binding documentation for lpc1850-creg-clk driver.
>>
>> Signed-off-by: Joachim Eastwood <manabian@gmail.com>
>> ---
>>  .../devicetree/bindings/clock/lpc1850-creg-clk.txt | 52 ++++++++++++++++++++++
>>  1 file changed, 52 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/clock/lpc1850-creg-clk.txt
>>
>> diff --git a/Documentation/devicetree/bindings/clock/lpc1850-creg-clk.txt b/Documentation/devicetree/bindings/clock/lpc1850-creg-clk.txt
>> new file mode 100644
>> index 000000000000..0c83d373b766
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/lpc1850-creg-clk.txt
>> @@ -0,0 +1,52 @@
>> +* NXP LPC1850 CREG clocks
>> +
>> +The NXP LPC18xx/43xx CREG (Configuration Registers) block contains
>> +control registers for two low speed clocks. One of the clocks is a
>> +32 kHz oscillator driver with power up/down and clock gating. Next
>> +is a fixed divider that creates a 1 kHz clock from the 32 kHz osc.
>> +
>> +These clocks are used by the RTC and the Event Router peripherials.
>> +The 32 kHz can also be routed to other peripherials to enable low
>> +power modes.
>> +
>> +This binding uses the common clock binding:
>> +    Documentation/devicetree/bindings/clock/clock-bindings.txt
>> +
>> +Required properties:
>> +- compatible:
>> +     Should be "nxp,lpc1850-creg-clk"
>> +- #clock-cells:
>> +     Shall have value <1>.
>> +- clocks:
>> +     Shall contain a phandle to the fix 32 kHz crystall.
>> +
>> +The creg-clk node must be a child of the creg syscon node.
>> +
>> +The following clocks are available from the clock node.
>> +
>> +Clock ID     Name
>> +   0          1 kHz clock
>> +   1         32 kHz Oscillator
>> +
>> +Example:
>> +soc {
>> +     creg: syscon@40043000 {
>> +             compatible = "nxp,lpc1850-creg", "syscon", "simple-mfd";
>> +             reg = <0x40043000 0x1000>;
>> +
>> +             creg_clk: clock-controller@004 {
>
> What is 4? A unit address should have a corresponding reg prop. and no
> leading zeros.

It is the fourth register in the creg syscon block.

Does this look better:
creg_clk: clock-controller@4 {
    compatible = "nxp,lpc1850-creg-clk";
    reg = <0x4>;
    clocks = <&xtal32>;
    #clock-cells = <1>;
};

Note that the reg property isn't really useful in the driver, but I
guess we should have it either way(?).

Alternatively I could drop the unit address and reg property. What do you think?


regards,
Joachim Eastwood

Re: [PATCH v2 2/2] doc: dt: add documentation for lpc1850-creg-clk driver

[Joachim Eastwood]

On 23 February 2016 at 19:08, Rob Herring <robh@kernel.org> wrote:
> On Sat, Feb 20, 2016 at 07:53:37PM +0100, Joachim Eastwood wrote:
>> Add DT binding documentation for lpc1850-creg-clk driver.
>>
>> Signed-off-by: Joachim Eastwood <manabian@gmail.com>
>> ---
>>  .../devicetree/bindings/clock/lpc1850-creg-clk.txt | 52 ++++++++++++++++++++++
>>  1 file changed, 52 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/clock/lpc1850-creg-clk.txt
>>
>> diff --git a/Documentation/devicetree/bindings/clock/lpc1850-creg-clk.txt b/Documentation/devicetree/bindings/clock/lpc1850-creg-clk.txt
>> new file mode 100644
>> index 000000000000..0c83d373b766
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/lpc1850-creg-clk.txt
>> @@ -0,0 +1,52 @@
>> +* NXP LPC1850 CREG clocks
>> +
>> +The NXP LPC18xx/43xx CREG (Configuration Registers) block contains
>> +control registers for two low speed clocks. One of the clocks is a
>> +32 kHz oscillator driver with power up/down and clock gating. Next
>> +is a fixed divider that creates a 1 kHz clock from the 32 kHz osc.
>> +
>> +These clocks are used by the RTC and the Event Router peripherials.
>> +The 32 kHz can also be routed to other peripherials to enable low
>> +power modes.
>> +
>> +This binding uses the common clock binding:
>> +    Documentation/devicetree/bindings/clock/clock-bindings.txt
>> +
>> +Required properties:
>> +- compatible:
>> +     Should be "nxp,lpc1850-creg-clk"
>> +- #clock-cells:
>> +     Shall have value <1>.
>> +- clocks:
>> +     Shall contain a phandle to the fix 32 kHz crystall.
>> +
>> +The creg-clk node must be a child of the creg syscon node.
>> +
>> +The following clocks are available from the clock node.
>> +
>> +Clock ID     Name
>> +   0          1 kHz clock
>> +   1         32 kHz Oscillator
>> +
>> +Example:
>> +soc {
>> +     creg: syscon@40043000 {
>> +             compatible = "nxp,lpc1850-creg", "syscon", "simple-mfd";
>> +             reg = <0x40043000 0x1000>;
>> +
>> +             creg_clk: clock-controller@004 {
>
> What is 4? A unit address should have a corresponding reg prop. and no
> leading zeros.

It is the fourth register in the creg syscon block.

Does this look better:
creg_clk: clock-controller@4 {
    compatible = "nxp,lpc1850-creg-clk";
    reg = <0x4>;
    clocks = <&xtal32>;
    #clock-cells = <1>;
};

Note that the reg property isn't really useful in the driver, but I
guess we should have it either way(?).

Alternatively I could drop the unit address and reg property. What do you think?


regards,
Joachim Eastwood

Re: [PATCH v2 2/2] doc: dt: add documentation for lpc1850-creg-clk driver

[Rob Herring]

On Tue, Feb 23, 2016 at 12:53 PM, Joachim  Eastwood <manabian@gmail.com> wrote:
> On 23 February 2016 at 19:08, Rob Herring <robh@kernel.org> wrote:
>> On Sat, Feb 20, 2016 at 07:53:37PM +0100, Joachim Eastwood wrote:
>>> Add DT binding documentation for lpc1850-creg-clk driver.
>>>
>>> Signed-off-by: Joachim Eastwood <manabian@gmail.com>
>>> ---
>>>  .../devicetree/bindings/clock/lpc1850-creg-clk.txt | 52 ++++++++++++++++++++++
>>>  1 file changed, 52 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/clock/lpc1850-creg-clk.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/clock/lpc1850-creg-clk.txt b/Documentation/devicetree/bindings/clock/lpc1850-creg-clk.txt
>>> new file mode 100644
>>> index 000000000000..0c83d373b766
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/clock/lpc1850-creg-clk.txt
>>> @@ -0,0 +1,52 @@
>>> +* NXP LPC1850 CREG clocks
>>> +
>>> +The NXP LPC18xx/43xx CREG (Configuration Registers) block contains
>>> +control registers for two low speed clocks. One of the clocks is a
>>> +32 kHz oscillator driver with power up/down and clock gating. Next
>>> +is a fixed divider that creates a 1 kHz clock from the 32 kHz osc.
>>> +
>>> +These clocks are used by the RTC and the Event Router peripherials.
>>> +The 32 kHz can also be routed to other peripherials to enable low
>>> +power modes.
>>> +
>>> +This binding uses the common clock binding:
>>> +    Documentation/devicetree/bindings/clock/clock-bindings.txt
>>> +
>>> +Required properties:
>>> +- compatible:
>>> +     Should be "nxp,lpc1850-creg-clk"
>>> +- #clock-cells:
>>> +     Shall have value <1>.
>>> +- clocks:
>>> +     Shall contain a phandle to the fix 32 kHz crystall.
>>> +
>>> +The creg-clk node must be a child of the creg syscon node.
>>> +
>>> +The following clocks are available from the clock node.
>>> +
>>> +Clock ID     Name
>>> +   0          1 kHz clock
>>> +   1         32 kHz Oscillator
>>> +
>>> +Example:
>>> +soc {
>>> +     creg: syscon@40043000 {
>>> +             compatible = "nxp,lpc1850-creg", "syscon", "simple-mfd";
>>> +             reg = <0x40043000 0x1000>;
>>> +
>>> +             creg_clk: clock-controller@004 {
>>
>> What is 4? A unit address should have a corresponding reg prop. and no
>> leading zeros.
>
> It is the fourth register in the creg syscon block.

Probably should be the offset rather than register count.

> Does this look better:
> creg_clk: clock-controller@4 {
>     compatible = "nxp,lpc1850-creg-clk";
>     reg = <0x4>;
>     clocks = <&xtal32>;
>     #clock-cells = <1>;
> };
>
> Note that the reg property isn't really useful in the driver, but I
> guess we should have it either way(?).
>
> Alternatively I could drop the unit address and reg property. What do you think?

Either way is fine. Though I would expect something needs to know the offset.

Rob

Re: [PATCH v2 2/2] doc: dt: add documentation for lpc1850-creg-clk driver

[Joachim Eastwood]

On 23 February 2016 at 22:58, Rob Herring <robh@kernel.org> wrote:
> On Tue, Feb 23, 2016 at 12:53 PM, Joachim  Eastwood <manabian@gmail.com> wrote:
>> On 23 February 2016 at 19:08, Rob Herring <robh@kernel.org> wrote:
>>> On Sat, Feb 20, 2016 at 07:53:37PM +0100, Joachim Eastwood wrote:
>>>> Add DT binding documentation for lpc1850-creg-clk driver.
>>>>
>>>> Signed-off-by: Joachim Eastwood <manabian@gmail.com>
>>>> ---
>>>>  .../devicetree/bindings/clock/lpc1850-creg-clk.txt | 52 ++++++++++++++++++++++
>>>>  1 file changed, 52 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/clock/lpc1850-creg-clk.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/clock/lpc1850-creg-clk.txt b/Documentation/devicetree/bindings/clock/lpc1850-creg-clk.txt
>>>> new file mode 100644
>>>> index 000000000000..0c83d373b766
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/clock/lpc1850-creg-clk.txt
>>>> @@ -0,0 +1,52 @@
>>>> +* NXP LPC1850 CREG clocks
>>>> +
>>>> +The NXP LPC18xx/43xx CREG (Configuration Registers) block contains
>>>> +control registers for two low speed clocks. One of the clocks is a
>>>> +32 kHz oscillator driver with power up/down and clock gating. Next
>>>> +is a fixed divider that creates a 1 kHz clock from the 32 kHz osc.
>>>> +
>>>> +These clocks are used by the RTC and the Event Router peripherials.
>>>> +The 32 kHz can also be routed to other peripherials to enable low
>>>> +power modes.
>>>> +
>>>> +This binding uses the common clock binding:
>>>> +    Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>> +
>>>> +Required properties:
>>>> +- compatible:
>>>> +     Should be "nxp,lpc1850-creg-clk"
>>>> +- #clock-cells:
>>>> +     Shall have value <1>.
>>>> +- clocks:
>>>> +     Shall contain a phandle to the fix 32 kHz crystall.
>>>> +
>>>> +The creg-clk node must be a child of the creg syscon node.
>>>> +
>>>> +The following clocks are available from the clock node.
>>>> +
>>>> +Clock ID     Name
>>>> +   0          1 kHz clock
>>>> +   1         32 kHz Oscillator
>>>> +
>>>> +Example:
>>>> +soc {
>>>> +     creg: syscon@40043000 {
>>>> +             compatible = "nxp,lpc1850-creg", "syscon", "simple-mfd";
>>>> +             reg = <0x40043000 0x1000>;
>>>> +
>>>> +             creg_clk: clock-controller@004 {
>>>
>>> What is 4? A unit address should have a corresponding reg prop. and no
>>> leading zeros.
>>
>> It is the fourth register in the creg syscon block.
>
> Probably should be the offset rather than register count.
>
>> Does this look better:
>> creg_clk: clock-controller@4 {
>>     compatible = "nxp,lpc1850-creg-clk";
>>     reg = <0x4>;
>>     clocks = <&xtal32>;
>>     #clock-cells = <1>;
>> };
>>
>> Note that the reg property isn't really useful in the driver, but I
>> guess we should have it either way(?).
>>
>> Alternatively I could drop the unit address and reg property. What do you think?
>
> Either way is fine. Though I would expect something needs to know the offset.

I have the offset in the driver itself. I don't except the driver to
reusable since the CREG block is highly SoC specific. If a new user
came along we could also place the register offset in the driver's
of_match_data.

I'll remove the unit address in the next version.

I have a couple of other device under the CREG that has unit address
already. I'll get them cleaned up as well.


Thanks for looking through the binding, Rob!


regards,
Joachim Eastwood

Re: [PATCH v2 2/2] doc: dt: add documentation for lpc1850-creg-clk driver

[Joachim Eastwood]

On 23 February 2016 at 22:58, Rob Herring <robh@kernel.org> wrote:
> On Tue, Feb 23, 2016 at 12:53 PM, Joachim  Eastwood <manabian@gmail.com> wrote:
>> On 23 February 2016 at 19:08, Rob Herring <robh@kernel.org> wrote:
>>> On Sat, Feb 20, 2016 at 07:53:37PM +0100, Joachim Eastwood wrote:
>>>> Add DT binding documentation for lpc1850-creg-clk driver.
>>>>
>>>> Signed-off-by: Joachim Eastwood <manabian@gmail.com>
>>>> ---
>>>>  .../devicetree/bindings/clock/lpc1850-creg-clk.txt | 52 ++++++++++++++++++++++
>>>>  1 file changed, 52 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/clock/lpc1850-creg-clk.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/clock/lpc1850-creg-clk.txt b/Documentation/devicetree/bindings/clock/lpc1850-creg-clk.txt
>>>> new file mode 100644
>>>> index 000000000000..0c83d373b766
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/clock/lpc1850-creg-clk.txt
>>>> @@ -0,0 +1,52 @@
>>>> +* NXP LPC1850 CREG clocks
>>>> +
>>>> +The NXP LPC18xx/43xx CREG (Configuration Registers) block contains
>>>> +control registers for two low speed clocks. One of the clocks is a
>>>> +32 kHz oscillator driver with power up/down and clock gating. Next
>>>> +is a fixed divider that creates a 1 kHz clock from the 32 kHz osc.
>>>> +
>>>> +These clocks are used by the RTC and the Event Router peripherials.
>>>> +The 32 kHz can also be routed to other peripherials to enable low
>>>> +power modes.
>>>> +
>>>> +This binding uses the common clock binding:
>>>> +    Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>> +
>>>> +Required properties:
>>>> +- compatible:
>>>> +     Should be "nxp,lpc1850-creg-clk"
>>>> +- #clock-cells:
>>>> +     Shall have value <1>.
>>>> +- clocks:
>>>> +     Shall contain a phandle to the fix 32 kHz crystall.
>>>> +
>>>> +The creg-clk node must be a child of the creg syscon node.
>>>> +
>>>> +The following clocks are available from the clock node.
>>>> +
>>>> +Clock ID     Name
>>>> +   0          1 kHz clock
>>>> +   1         32 kHz Oscillator
>>>> +
>>>> +Example:
>>>> +soc {
>>>> +     creg: syscon@40043000 {
>>>> +             compatible = "nxp,lpc1850-creg", "syscon", "simple-mfd";
>>>> +             reg = <0x40043000 0x1000>;
>>>> +
>>>> +             creg_clk: clock-controller@004 {
>>>
>>> What is 4? A unit address should have a corresponding reg prop. and no
>>> leading zeros.
>>
>> It is the fourth register in the creg syscon block.
>
> Probably should be the offset rather than register count.
>
>> Does this look better:
>> creg_clk: clock-controller@4 {
>>     compatible = "nxp,lpc1850-creg-clk";
>>     reg = <0x4>;
>>     clocks = <&xtal32>;
>>     #clock-cells = <1>;
>> };
>>
>> Note that the reg property isn't really useful in the driver, but I
>> guess we should have it either way(?).
>>
>> Alternatively I could drop the unit address and reg property. What do you think?
>
> Either way is fine. Though I would expect something needs to know the offset.

I have the offset in the driver itself. I don't except the driver to
reusable since the CREG block is highly SoC specific. If a new user
came along we could also place the register offset in the driver's
of_match_data.

I'll remove the unit address in the next version.

I have a couple of other device under the CREG that has unit address
already. I'll get them cleaned up as well.


Thanks for looking through the binding, Rob!


regards,
Joachim Eastwood

Re: [PATCH v2 1/2] clk: add lpc18xx creg clk driver

[Michael Turquette]

Quoting Joachim Eastwood (2016-02-20 10:53:36)
> +static const struct clk_ops clk_creg_32k = {
> +       .enable         = clk_creg_enable,
> +       .disable        = clk_creg_disable,

There should be a .is_enabled here.

> +       .prepare        = clk_creg_32k_prepare,
> +       .unprepare      = clk_creg_32k_unprepare,
> +       .is_prepared    = clk_creg_32k_is_prepared,
> +};
> +
> +static const struct clk_ops clk_creg_1k = {
> +       .enable         = clk_creg_enable,
> +       .disable        = clk_creg_disable,

Ditto.

Otherwise patch looks good to me. Thanks a lot for all of the rework.

Regards,
Mike

Re: [PATCH v2 1/2] clk: add lpc18xx creg clk driver

[Michael Turquette]

Quoting Joachim Eastwood (2016-02-20 10:53:36)
> +static const struct clk_ops clk_creg_32k =3D {
> +       .enable         =3D clk_creg_enable,
> +       .disable        =3D clk_creg_disable,

There should be a .is_enabled here.

> +       .prepare        =3D clk_creg_32k_prepare,
> +       .unprepare      =3D clk_creg_32k_unprepare,
> +       .is_prepared    =3D clk_creg_32k_is_prepared,
> +};
> +
> +static const struct clk_ops clk_creg_1k =3D {
> +       .enable         =3D clk_creg_enable,
> +       .disable        =3D clk_creg_disable,

Ditto.

Otherwise patch looks good to me. Thanks a lot for all of the rework.

Regards,
Mike