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

[PATCH 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.

Based on v4.2-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                 | 173 +++++++++++++++++++++
 3 files changed, 226 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 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 | 173 +++++++++++++++++++++++++++++++++++++
 2 files changed, 174 insertions(+)
 create mode 100644 drivers/clk/nxp/clk-lpc18xx-creg.c

diff --git a/drivers/clk/nxp/Makefile b/drivers/clk/nxp/Makefile
index 7f608b0ad7b4..6f6b0f9aa320 100644
--- a/drivers/clk/nxp/Makefile
+++ b/drivers/clk/nxp/Makefile
@@ -1,2 +1,3 @@
 obj-$(CONFIG_ARCH_LPC18XX)	+= clk-lpc18xx-cgu.o
 obj-$(CONFIG_ARCH_LPC18XX)	+= clk-lpc18xx-ccu.o
+obj-$(CONFIG_ARCH_LPC18XX)	+= clk-lpc18xx-creg.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..4bca11eca4a1
--- /dev/null
+++ b/drivers/clk/nxp/clk-lpc18xx-creg.c
@@ -0,0 +1,173 @@
+/*
+ * 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/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] =
+		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);
+}
+CLK_OF_DECLARE(lpc18xx_creg_clk, "nxp,lpc1850-creg-clk", lpc18xx_creg_clk_init);
-- 
1.8.0

[PATCH 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 at 40043000 {
+		compatible = "nxp,lpc1850-creg", "syscon", "simple-mfd";
+		reg = <0x40043000 0x1000>;
+
+		creg_clk: clock-controller at 004 {
+			compatible = "nxp,lpc1850-creg-clk";
+			clocks = <&xtal32>;
+			#clock-cells = <1>;
+		};
+
+		...
+	};
+
+	rtc: rtc at 40046000 {
+		...
+		clocks = <&creg_clk 0>, <&ccu1 CLK_CPU_BUS>;
+		clock-names = "rtc", "reg";
+		...
+	};
+};
-- 
1.8.0

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

[Michael Turquette]

Hi Joachim,

Quoting Joachim Eastwood (2015-07-11 14:48:26)
> +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] =
> +               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);
> +}
> +CLK_OF_DECLARE(lpc18xx_creg_clk, "nxp,lpc1850-creg-clk", lpc18xx_creg_clk_init);

I'll ask the same question that Stephen asked in your CCU/CGU driver
series: is it necessary to use CLK_OF_DECLARE here or can you use the
platform device model?

Thanks,
Mike

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

[Joachim Eastwood]

On 11 August 2015 at 22:41, Michael Turquette <mturquette@baylibre.com> wrote:
> Hi Joachim,
>
> Quoting Joachim Eastwood (2015-07-11 14:48:26)
>> +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] =
>> +               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);
>> +}
>> +CLK_OF_DECLARE(lpc18xx_creg_clk, "nxp,lpc1850-creg-clk", lpc18xx_creg_clk_init);
>
> I'll ask the same question that Stephen asked in your CCU/CGU driver
> series: is it necessary to use CLK_OF_DECLARE here or can you use the
> platform device model?

The 32 kHz clock from the CREG block is a clock parent to the CGU
block so it's possible that it will required early. This is all
depends on how the boot loader initially configures the CGU.

Currently in the DTS for lpc18xx cgu it has:
clocks = <&xtal>, <&xtal32>, <...>;
xtal32 is just a temporary placeholder until the CREG clock is in place.


Note that while it's possible to use the 32k clock together with PLL0
to generate the main CPU bus clock I doubt it will be common
configuration. It is however fully possible to do.


regards,
Joachim Eastwood

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

[Michael Turquette]

Quoting Joachim Eastwood (2015-08-13 13:43:11)
> On 11 August 2015 at 22:41, Michael Turquette <mturquette@baylibre.com> wrote:
> > Hi Joachim,
> >
> > Quoting Joachim Eastwood (2015-07-11 14:48:26)
> >> +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] =
> >> +               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);
> >> +}
> >> +CLK_OF_DECLARE(lpc18xx_creg_clk, "nxp,lpc1850-creg-clk", lpc18xx_creg_clk_init);
> >
> > I'll ask the same question that Stephen asked in your CCU/CGU driver
> > series: is it necessary to use CLK_OF_DECLARE here or can you use the
> > platform device model?
> 
> The 32 kHz clock from the CREG block is a clock parent to the CGU
> block so it's possible that it will required early. This is all
> depends on how the boot loader initially configures the CGU.
> 
> Currently in the DTS for lpc18xx cgu it has:
> clocks = <&xtal>, <&xtal32>, <...>;
> xtal32 is just a temporary placeholder until the CREG clock is in place.

Well that seems wrong. Is it just a matter of probe order where you try
to probe the cgu driver before the creg driver?

Regards,
Mike

> 
> 
> Note that while it's possible to use the 32k clock together with PLL0
> to generate the main CPU bus clock I doubt it will be common
> configuration. It is however fully possible to do.
> 
> 
> regards,
> Joachim Eastwood
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Stephen Boyd]

On 07/11, Joachim Eastwood wrote:
> 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.
> 

I know this is many months old, but is this driver still
required? I can apply it to clk-next if so.

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

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

[Joachim Eastwood]

Hi Stephen,

On 8 February 2016 at 23:26, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 07/11, Joachim Eastwood wrote:
>> 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.
>>
>
> I know this is many months old, but is this driver still
> required? I can apply it to clk-next if so.

It is still needed for the internal RTC (rtc-lpc24xx) on lpc18xx. The
RTC uses a 1 kHz clock that is controlled in CREG (misc system regs).

Note; if you apply it you will most likely get a very trivial conflict
in drivers/clk/nxp/Makefile because clk-lpc32xx.c has been added since
these patches was sent.


regards,
Joachim Eastwood

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

[Stephen Boyd]

On 02/09, Joachim Eastwood wrote:
> Hi Stephen,
> 
> On 8 February 2016 at 23:26, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > On 07/11, Joachim Eastwood wrote:
> >> 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.
> >>
> >
> > I know this is many months old, but is this driver still
> > required? I can apply it to clk-next if so.
> 
> It is still needed for the internal RTC (rtc-lpc24xx) on lpc18xx. The
> RTC uses a 1 kHz clock that is controlled in CREG (misc system regs).
> 
> Note; if you apply it you will most likely get a very trivial conflict
> in drivers/clk/nxp/Makefile because clk-lpc32xx.c has been added since
> these patches was sent.
> 

Hm, ok. I see that Mike had some comments on the thread that look
to be left unanswered? Care to address those first?

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

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

[Joachim Eastwood]

Hi Mike,

Seems your reply got lost in my mailbox and I didn't notice it before
Stephen replied on the cover letter. Sorry about that.

On 18 August 2015 at 02:26, Michael Turquette <mturquette@baylibre.com> wrote:
> Quoting Joachim Eastwood (2015-08-13 13:43:11)
>> On 11 August 2015 at 22:41, Michael Turquette <mturquette@baylibre.com> wrote:
>> > Hi Joachim,
>> >
>> > Quoting Joachim Eastwood (2015-07-11 14:48:26)
>> >> +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] =
>> >> +               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);
>> >> +}
>> >> +CLK_OF_DECLARE(lpc18xx_creg_clk, "nxp,lpc1850-creg-clk", lpc18xx_creg_clk_init);
>> >
>> > I'll ask the same question that Stephen asked in your CCU/CGU driver
>> > series: is it necessary to use CLK_OF_DECLARE here or can you use the
>> > platform device model?
>>
>> The 32 kHz clock from the CREG block is a clock parent to the CGU
>> block so it's possible that it will required early. This is all
>> depends on how the boot loader initially configures the CGU.
>>
>> Currently in the DTS for lpc18xx cgu it has:
>> clocks = <&xtal>, <&xtal32>, <...>;
>> xtal32 is just a temporary placeholder until the CREG clock is in place.
>
> Well that seems wrong. Is it just a matter of probe order where you try
> to probe the cgu driver before the creg driver?

The ideal probe order for the clk drivers on the lpc18xx platform
would be; creg-clk, cgu and ccu.
If the 32k clk is used by any of timers we need creg-clk to enable the
32k clock and determine the rate.

Note that the cgu and ccu driver is using CLK_OF_DECLARE now.


I have tired to create an overview of the lpc18xx clock system here:
https://github.com/manabian/linux-lpc/wiki/LPC18xx-LPC43xx-clocks


regards,
Joachim Eastwood

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

[Michael Turquette]

Quoting Joachim Eastwood (2016-02-13 06:38:45)
> Hi Mike,
> 
> Seems your reply got lost in my mailbox and I didn't notice it before
> Stephen replied on the cover letter. Sorry about that.
> 
> On 18 August 2015 at 02:26, Michael Turquette <mturquette@baylibre.com> wrote:
> > Quoting Joachim Eastwood (2015-08-13 13:43:11)
> >> On 11 August 2015 at 22:41, Michael Turquette <mturquette@baylibre.com> wrote:
> >> > Hi Joachim,
> >> >
> >> > Quoting Joachim Eastwood (2015-07-11 14:48:26)
> >> >> +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] =
> >> >> +               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);
> >> >> +}
> >> >> +CLK_OF_DECLARE(lpc18xx_creg_clk, "nxp,lpc1850-creg-clk", lpc18xx_creg_clk_init);
> >> >
> >> > I'll ask the same question that Stephen asked in your CCU/CGU driver
> >> > series: is it necessary to use CLK_OF_DECLARE here or can you use the
> >> > platform device model?
> >>
> >> The 32 kHz clock from the CREG block is a clock parent to the CGU
> >> block so it's possible that it will required early. This is all
> >> depends on how the boot loader initially configures the CGU.
> >>
> >> Currently in the DTS for lpc18xx cgu it has:
> >> clocks = <&xtal>, <&xtal32>, <...>;
> >> xtal32 is just a temporary placeholder until the CREG clock is in place.
> >
> > Well that seems wrong. Is it just a matter of probe order where you try
> > to probe the cgu driver before the creg driver?
> 
> The ideal probe order for the clk drivers on the lpc18xx platform
> would be; creg-clk, cgu and ccu.
> If the 32k clk is used by any of timers we need creg-clk to enable the
> 32k clock and determine the rate.
> 
> Note that the cgu and ccu driver is using CLK_OF_DECLARE now.
> 
> 
> I have tired to create an overview of the lpc18xx clock system here:
> https://github.com/manabian/linux-lpc/wiki/LPC18xx-LPC43xx-clocks

That's great, thanks a lot for the link and the nice documentation.

It's been a while since I've looked at this thread. Do any of the
creg-clk, cgu or ccu clock drivers need to use CLK_OF_DECLARE? If they
were platform_drivers then you could use -EPROBE_DEFER to solve your
ordering issue.

Regards,
Mike

> 
> 
> regards,
> Joachim Eastwood

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

[Joachim Eastwood]

On 17 February 2016 at 01:56, Michael Turquette <mturquette@baylibre.com> wrote:
> Quoting Joachim Eastwood (2016-02-13 06:38:45)
>> Hi Mike,
>>
>> Seems your reply got lost in my mailbox and I didn't notice it before
>> Stephen replied on the cover letter. Sorry about that.
>>
>> On 18 August 2015 at 02:26, Michael Turquette <mturquette@baylibre.com> wrote:
>> > Quoting Joachim Eastwood (2015-08-13 13:43:11)
>> >> On 11 August 2015 at 22:41, Michael Turquette <mturquette@baylibre.com> wrote:
>> >> > Hi Joachim,
>> >> >
>> >> > Quoting Joachim Eastwood (2015-07-11 14:48:26)
>> >> >> +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] =
>> >> >> +               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);
>> >> >> +}
>> >> >> +CLK_OF_DECLARE(lpc18xx_creg_clk, "nxp,lpc1850-creg-clk", lpc18xx_creg_clk_init);
>> >> >
>> >> > I'll ask the same question that Stephen asked in your CCU/CGU driver
>> >> > series: is it necessary to use CLK_OF_DECLARE here or can you use the
>> >> > platform device model?
>> >>
>> >> The 32 kHz clock from the CREG block is a clock parent to the CGU
>> >> block so it's possible that it will required early. This is all
>> >> depends on how the boot loader initially configures the CGU.
>> >>
>> >> Currently in the DTS for lpc18xx cgu it has:
>> >> clocks = <&xtal>, <&xtal32>, <...>;
>> >> xtal32 is just a temporary placeholder until the CREG clock is in place.
>> >
>> > Well that seems wrong. Is it just a matter of probe order where you try
>> > to probe the cgu driver before the creg driver?
>>
>> The ideal probe order for the clk drivers on the lpc18xx platform
>> would be; creg-clk, cgu and ccu.
>> If the 32k clk is used by any of timers we need creg-clk to enable the
>> 32k clock and determine the rate.
>>
>> Note that the cgu and ccu driver is using CLK_OF_DECLARE now.
>>
>>
>> I have tired to create an overview of the lpc18xx clock system here:
>> https://github.com/manabian/linux-lpc/wiki/LPC18xx-LPC43xx-clocks
>
> That's great, thanks a lot for the link and the nice documentation.
>
> It's been a while since I've looked at this thread. Do any of the
> creg-clk, cgu or ccu clock drivers need to use CLK_OF_DECLARE? If they
> were platform_drivers then you could use -EPROBE_DEFER to solve your
> ordering issue.

The clock for the clocksource/timer
(drivers/clocksource/time-lpc32xx.c) comes from cgu/ccu and can also
come from creg-clk. So afaik they must use CLK_OF_DECLARE. Or is there
something I am missing?
Isn't true that most SoC system clock drivers must use CLK_OF_DECLARE
because the system timer require clocks early? I thought this was the
whole point.

Since it is possible that the 32k clock from creg-clk can be use as a
parent clock for the timer doesn't this mean it also must use
CLK_OF_DECLARE?
If it wasn't possible for creg-clk to be used as a parent clock for
the timer it could have been a platform device.


The lpc18xx clock configuration that require creg-clk early would be:
32k - > PLL0 -> BASE_M3/M4_CLK -> CLK_M3/M4_TIMER0. See second figure
on the webpage. Note that this is probably not a common configuration,
but it is a valid one. (PLL0 accepts input down to 14 kHz)


regards,
Joachim Eastwood

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

[Michael Turquette]

Quoting Joachim Eastwood (2016-02-17 10:24:12)
> On 17 February 2016 at 01:56, Michael Turquette <mturquette@baylibre.com> wrote:
> > Quoting Joachim Eastwood (2016-02-13 06:38:45)
> >> Hi Mike,
> >>
> >> Seems your reply got lost in my mailbox and I didn't notice it before
> >> Stephen replied on the cover letter. Sorry about that.
> >>
> >> On 18 August 2015 at 02:26, Michael Turquette <mturquette@baylibre.com> wrote:
> >> > Quoting Joachim Eastwood (2015-08-13 13:43:11)
> >> >> On 11 August 2015 at 22:41, Michael Turquette <mturquette@baylibre.com> wrote:
> >> >> > Hi Joachim,
> >> >> >
> >> >> > Quoting Joachim Eastwood (2015-07-11 14:48:26)
> >> >> >> +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] =
> >> >> >> +               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);
> >> >> >> +}
> >> >> >> +CLK_OF_DECLARE(lpc18xx_creg_clk, "nxp,lpc1850-creg-clk", lpc18xx_creg_clk_init);
> >> >> >
> >> >> > I'll ask the same question that Stephen asked in your CCU/CGU driver
> >> >> > series: is it necessary to use CLK_OF_DECLARE here or can you use the
> >> >> > platform device model?
> >> >>
> >> >> The 32 kHz clock from the CREG block is a clock parent to the CGU
> >> >> block so it's possible that it will required early. This is all
> >> >> depends on how the boot loader initially configures the CGU.
> >> >>
> >> >> Currently in the DTS for lpc18xx cgu it has:
> >> >> clocks = <&xtal>, <&xtal32>, <...>;
> >> >> xtal32 is just a temporary placeholder until the CREG clock is in place.
> >> >
> >> > Well that seems wrong. Is it just a matter of probe order where you try
> >> > to probe the cgu driver before the creg driver?
> >>
> >> The ideal probe order for the clk drivers on the lpc18xx platform
> >> would be; creg-clk, cgu and ccu.
> >> If the 32k clk is used by any of timers we need creg-clk to enable the
> >> 32k clock and determine the rate.
> >>
> >> Note that the cgu and ccu driver is using CLK_OF_DECLARE now.
> >>
> >>
> >> I have tired to create an overview of the lpc18xx clock system here:
> >> https://github.com/manabian/linux-lpc/wiki/LPC18xx-LPC43xx-clocks
> >
> > That's great, thanks a lot for the link and the nice documentation.
> >
> > It's been a while since I've looked at this thread. Do any of the
> > creg-clk, cgu or ccu clock drivers need to use CLK_OF_DECLARE? If they
> > were platform_drivers then you could use -EPROBE_DEFER to solve your
> > ordering issue.
> 
> The clock for the clocksource/timer
> (drivers/clocksource/time-lpc32xx.c) comes from cgu/ccu and can also
> come from creg-clk. So afaik they must use CLK_OF_DECLARE. Or is there
> something I am missing?

No, you're not missing anything. What I want is for clock provider
drivers to actually be real Linux device drivers. Right now the drivers
that only call CLK_OF_DECLARE are essentially just initcall hacks, and
this causes problems down the road when you decide you want your clk
provider driver to have suspend/resume ops, etc. Also I feel that having
proper drivers will be important as runtime pm and the generic power
domains stuff matures.

> Isn't true that most SoC system clock drivers must use CLK_OF_DECLARE
> because the system timer require clocks early? I thought this was the
> whole point.

On ARMv8, mandatory use of architected timers removes the need for the
clk framework to be involved with timer init, so it's definitely not
true for all platforms.

On QCOM's 32-bit platforms they have an always-on clock with a known
fixed rate, so they can either put the fixed-rate clock in DT, or even
just put the frequency as a property into the timer node with no need to
involve the clk framework (which is what they do today). That is also a
bit hacky, but it is why you won't find CLK_OF_DECLARE in
drivers/clk/qcom and all of their drivers are proper platform_drivers.

> 
> Since it is possible that the 32k clock from creg-clk can be use as a
> parent clock for the timer doesn't this mean it also must use
> CLK_OF_DECLARE?

Right, if your 32k clock is a fixed-clock, and if it is provided on the
board (e.g. external extal -> lpc timer) then you could just put the
fixed-rate clock into DT (which uses CLK_OF_DECLARE) and reference that
from your timer node. The rest of the clocks could be registered later
through a platform_driver.

Sounds like your timer clocking scheme is more complicated than that
however.

> If it wasn't possible for creg-clk to be used as a parent clock for
> the timer it could have been a platform device.
> 
> 
> The lpc18xx clock configuration that require creg-clk early would be:
> 32k - > PLL0 -> BASE_M3/M4_CLK -> CLK_M3/M4_TIMER0. See second figure
> on the webpage. Note that this is probably not a common configuration,
> but it is a valid one. (PLL0 accepts input down to 14 kHz)

I was just discussing this with Stephen and we're both curious to know
if the same "nxp,lpc1850-creg-clk" compatible string can be used with
CLK_OF_DECLARE for registering the bare minimum early clks, and also
with a platform_driver using a different table of clocks for late
registration.

The compat string could be the same, and the driver would have different
tables/setup functions to keep from re-registering the same clocks
twice.

I know that nobody likes to be the guinea pig for this type of stuff,
but the lack of participation in the Linux driver model for clk drivers
needs to curbed. Do you think the above proposed scheme could work for
your clock provider drivers?

Regards,
Mike

> 
> 
> regards,
> Joachim Eastwood

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

[Joachim Eastwood]

On 17 February 2016 at 21:28, Michael Turquette <mturquette@baylibre.com> wrote:
> Quoting Joachim Eastwood (2016-02-17 10:24:12)
>> On 17 February 2016 at 01:56, Michael Turquette <mturquette@baylibre.com> wrote:
>> > Quoting Joachim Eastwood (2016-02-13 06:38:45)
>> >> Hi Mike,
>> >>
>> >> Seems your reply got lost in my mailbox and I didn't notice it before
>> >> Stephen replied on the cover letter. Sorry about that.
>> >>
>> >> On 18 August 2015 at 02:26, Michael Turquette <mturquette@baylibre.com> wrote:
>> >> > Quoting Joachim Eastwood (2015-08-13 13:43:11)
>> >> >> On 11 August 2015 at 22:41, Michael Turquette <mturquette@baylibre.com> wrote:
>> >> >> > Hi Joachim,
>> >> >> >
>> >> >> > Quoting Joachim Eastwood (2015-07-11 14:48:26)
>> >> >> >> +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] =
>> >> >> >> +               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);
>> >> >> >> +}
>> >> >> >> +CLK_OF_DECLARE(lpc18xx_creg_clk, "nxp,lpc1850-creg-clk", lpc18xx_creg_clk_init);
>> >> >> >
>> >> >> > I'll ask the same question that Stephen asked in your CCU/CGU driver
>> >> >> > series: is it necessary to use CLK_OF_DECLARE here or can you use the
>> >> >> > platform device model?
>> >> >>
>> >> >> The 32 kHz clock from the CREG block is a clock parent to the CGU
>> >> >> block so it's possible that it will required early. This is all
>> >> >> depends on how the boot loader initially configures the CGU.
>> >> >>
>> >> >> Currently in the DTS for lpc18xx cgu it has:
>> >> >> clocks = <&xtal>, <&xtal32>, <...>;
>> >> >> xtal32 is just a temporary placeholder until the CREG clock is in place.
>> >> >
>> >> > Well that seems wrong. Is it just a matter of probe order where you try
>> >> > to probe the cgu driver before the creg driver?
>> >>
>> >> The ideal probe order for the clk drivers on the lpc18xx platform
>> >> would be; creg-clk, cgu and ccu.
>> >> If the 32k clk is used by any of timers we need creg-clk to enable the
>> >> 32k clock and determine the rate.
>> >>
>> >> Note that the cgu and ccu driver is using CLK_OF_DECLARE now.
>> >>
>> >>
>> >> I have tired to create an overview of the lpc18xx clock system here:
>> >> https://github.com/manabian/linux-lpc/wiki/LPC18xx-LPC43xx-clocks
>> >
>> > That's great, thanks a lot for the link and the nice documentation.
>> >
>> > It's been a while since I've looked at this thread. Do any of the
>> > creg-clk, cgu or ccu clock drivers need to use CLK_OF_DECLARE? If they
>> > were platform_drivers then you could use -EPROBE_DEFER to solve your
>> > ordering issue.
>>
>> The clock for the clocksource/timer
>> (drivers/clocksource/time-lpc32xx.c) comes from cgu/ccu and can also
>> come from creg-clk. So afaik they must use CLK_OF_DECLARE. Or is there
>> something I am missing?
>
> No, you're not missing anything. What I want is for clock provider
> drivers to actually be real Linux device drivers. Right now the drivers
> that only call CLK_OF_DECLARE are essentially just initcall hacks, and
> this causes problems down the road when you decide you want your clk
> provider driver to have suspend/resume ops, etc. Also I feel that having
> proper drivers will be important as runtime pm and the generic power
> domains stuff matures.
>
>> Isn't true that most SoC system clock drivers must use CLK_OF_DECLARE
>> because the system timer require clocks early? I thought this was the
>> whole point.
>
> On ARMv8, mandatory use of architected timers removes the need for the
> clk framework to be involved with timer init, so it's definitely not
> true for all platforms.
>
> On QCOM's 32-bit platforms they have an always-on clock with a known
> fixed rate, so they can either put the fixed-rate clock in DT, or even
> just put the frequency as a property into the timer node with no need to
> involve the clk framework (which is what they do today). That is also a
> bit hacky, but it is why you won't find CLK_OF_DECLARE in
> drivers/clk/qcom and all of their drivers are proper platform_drivers.
>
>>
>> Since it is possible that the 32k clock from creg-clk can be use as a
>> parent clock for the timer doesn't this mean it also must use
>> CLK_OF_DECLARE?
>
> Right, if your 32k clock is a fixed-clock, and if it is provided on the
> board (e.g. external extal -> lpc timer) then you could just put the
> fixed-rate clock into DT (which uses CLK_OF_DECLARE) and reference that
> from your timer node. The rest of the clocks could be registered later
> through a platform_driver.
>
> Sounds like your timer clocking scheme is more complicated than that
> however.
>
>> If it wasn't possible for creg-clk to be used as a parent clock for
>> the timer it could have been a platform device.
>>
>>
>> The lpc18xx clock configuration that require creg-clk early would be:
>> 32k - > PLL0 -> BASE_M3/M4_CLK -> CLK_M3/M4_TIMER0. See second figure
>> on the webpage. Note that this is probably not a common configuration,
>> but it is a valid one. (PLL0 accepts input down to 14 kHz)
>
> I was just discussing this with Stephen and we're both curious to know
> if the same "nxp,lpc1850-creg-clk" compatible string can be used with
> CLK_OF_DECLARE for registering the bare minimum early clks, and also
> with a platform_driver using a different table of clocks for late
> registration.
>
> The compat string could be the same, and the driver would have different
> tables/setup functions to keep from re-registering the same clocks
> twice.

creg-clk is quite simple with only two clocks and only one of them may
be needed during boot.

> I know that nobody likes to be the guinea pig for this type of stuff,
> but the lack of participation in the Linux driver model for clk drivers
> needs to curbed. Do you think the above proposed scheme could work for
> your clock provider drivers?

For creg-clk it would probably work.

cgu would need to register most of clocks early since it needs to
support whatever setup the bootloader throws at it. The only clocks
that could be taken out is the base clocks (clk_onecell_data
clk_base_data) except for one of them. But splitting this table would
be a bad idea as hw clk numbers are used as indexes for easy look up.

Come to think of it; isn't splitting the clock table between a early
init and platform driver going become a problem?
At least the lookup in the driver or framework would become more complex(?)
And won't you confuse the lookup?
If the early driver register one clock table with
of_clk_add_provider() and then sometime later the platform driver
register another clock table, both using the same of_node. How is that
suppose to work?

ccu has only 4 clocks needed early; CLK_CPU_TIMERx with x = 0..3.


Note that I don't have much time to spare on lpc18xx right now so I
might be a bit slow if you want me to implement and test stuff.


regards,
Joachim Eastwood

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

[Stephen Boyd]

On 02/17, Joachim Eastwood wrote:
> 
> For creg-clk it would probably work.
> 
> cgu would need to register most of clocks early since it needs to
> support whatever setup the bootloader throws at it. The only clocks
> that could be taken out is the base clocks (clk_onecell_data
> clk_base_data) except for one of them. But splitting this table would
> be a bad idea as hw clk numbers are used as indexes for easy look up.
> 
> Come to think of it; isn't splitting the clock table between a early
> init and platform driver going become a problem?
> At least the lookup in the driver or framework would become more complex(?)
> And won't you confuse the lookup?
> If the early driver register one clock table with
> of_clk_add_provider() and then sometime later the platform driver
> register another clock table, both using the same of_node. How is that
> suppose to work?

I believe that should work, although I haven't tested it. The
code looks until it finds a provider that matches the device node
and gives us a valid clk pointer. If we don't get a valid pointer
we keep looking for another provider until we exhaust all
providers.

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