clk: lpc32xx: add clock support for NXP LPC32xx

NXP LPC32xx SoC has a clocking and power control unit (CPC) as a part of system control block (SCB). CPC is supplied by two external oscillators and it manages core and most of peripheral clocks, the change adds description of DT bindings for clock controller found on LPC32xx SoC series.

Signed-off-by Vladimir Zapolskiy <[email protected]>

---
 .../devicetree/bindings/clock/nxp,lpc3220-clk.txt  | 30 ++++++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/nxp,lpc3220-clk.txt

Documentation/devicetree/bindings/clock/nxp,lpc3220-clk.txt new file +0 -0

new file mode 100644
index 0000000..20cbca3

Quoted 18 lines · On Friday 20 November 2015 03:05:01 Vladimir Zapolskiy wrote:

NXP LPC32xx SoC has a clocking and power control unit (CPC) as a part of system control block (SCB). CPC is supplied by two external oscillators and it manages core and most of peripheral clocks, the change adds description of DT bindings for clock controller found on LPC32xx SoC series.

Signed-off-by Vladimir Zapolskiy <[email protected]>

---
 .../devicetree/bindings/clock/nxp,lpc3220-clk.txt  | 30 ++++++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/nxp,lpc3220-clk.txt

Documentation/devicetree/bindings/clock/nxp,lpc3220-clk.txt new file +0 -0

new file mode 100644
index 0000000..20cbca3

Documentation/devicetree/bindings/clock/nxp,lpc3220-clk.txt line 4

Quoted 4 lines

Documentation/devicetree/bindings/clock/nxp,lpc3220-clk.txt new file +4 -0

@@ -0,0 +1,30 @@
+NXP LPC32xx Clock Controller
+
+Required properties:
+- compatible: should be "nxp,lpc3220-clk"

Please use a specific model number without 'xx' wildcards. If you have multiple chips that are mutually compatible, pick one as the base number and then list the others as more specific instances, like

	compatible = "nxp,lpc3250-clk", "nxp,lpc3220-clk";

Quoted 6 lines

Please use a specific model number without 'xx' wildcards. If you have multiple chips that are mutually compatible, pick one as the base number and then list the others as more specific instances, like

      compatible = "nxp,lpc3250-clk", "nxp,lpc3220-clk";

Do you ask me to change a title? You may see that compatible property does not contain any wildcards?

NXP LPC32xx USB controller has a subdevice, which controls USB AHB slave, USB OTG, USB OHCI, USB device and I2C controller to USB phy clocks, this change adds description of the clock controller, for more details reference LPC32xx User's Manual, namely USB control, OTG clock control and OTG clock status registers.

Signed-off-by Vladimir Zapolskiy <[email protected]>

---
 .../bindings/clock/nxp,lpc3220-usb-clk.txt         | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/nxp,lpc3220-usb-clk.txt

Documentation/devicetree/bindings/clock/nxp,lpc3220-usb-clk.txt new file +1 -0

new file mode 100644
index 0000000..67fba7f
@@ -0,0 +1,22 @@
+NXP LPC32xx USB Clock Controller

Documentation/devicetree/bindings/clock/nxp,lpc3220-usb-clk.txt new file +3 -0

+			#clock-cells = <1>;
+		};
+	};

Signature · 1 line

2.1.4

Quoted 24 lines · Vladimir Zapolskiy

NXP LPC32xx USB controller has a subdevice, which controls USB AHB slave, USB OTG, USB OHCI, USB device and I2C controller to USB phy clocks, this change adds description of the clock controller, for more details reference LPC32xx User's Manual, namely USB control, OTG clock control and OTG clock status registers.

Signed-off-by Vladimir Zapolskiy <[email protected]>

---
 .../bindings/clock/nxp,lpc3220-usb-clk.txt         | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/nxp,lpc3220-usb-clk.txt

Documentation/devicetree/bindings/clock/nxp,lpc3220-usb-clk.txt new file +6 -0

new file mode 100644
index 0000000..67fba7f
@@ -0,0 +1,22 @@
+NXP LPC32xx USB Clock Controller
+
+Required properties:
+- compatible: should be "nxp,lpc3220-usb-clk"
+- reg:  should contain clock controller registers location and length
+- #clock-cells: must be 1, the cell holds id of a clock provided by the

Documentation/devicetree/bindings/clock/nxp,lpc3220-usb-clk.txt line 11

Quoted 5 lines

Documentation/devicetree/bindings/clock/nxp,lpc3220-usb-clk.txt new file +5 -0

+  USB clock controller
+
+Examples:
+
+	usb {

I don't understand the full structure of USB blocks. Can you make the example complete. All the blocks are a child of this node?

Documentation/devicetree/bindings/clock/nxp,lpc3220-usb-clk.txt line 17

Quoted 6 lines

Documentation/devicetree/bindings/clock/nxp,lpc3220-usb-clk.txt new file +6 -0

+		#address-cells = <1>;
+		#size-cells = <1>;
+		compatible = "simple-bus";
+		ranges = <0x0 0x31020000 0x00001000>;
+
+		usbclk: clock-controller at F00 {

lower case ^

Documentation/devicetree/bindings/clock/nxp,lpc3220-usb-clk.txt line 19

Quoted 2 lines

Documentation/devicetree/bindings/clock/nxp,lpc3220-usb-clk.txt new file +2 -0

+			compatible = "nxp,lpc3220-usb-clk";
+			reg = <0xF00 0x100>;

lower case

Quoted 6 lines

Documentation/devicetree/bindings/clock/nxp,lpc3220-usb-clk.txt new file +3 -0

+			#clock-cells = <1>;
+		};
+	};

Signature · 1 line

2.1.4

include/dt-bindings/clock/lpc32xx-clock.h new file +8 -0

+
+/* LPC32XX System Control Block clocks */
+#define LPC32XX_CLK_RTC                0
+#define LPC32XX_CLK_DMA                1
+#define LPC32XX_CLK_MLC                2
+#define LPC32XX_CLK_SLC                3
+#define LPC32XX_CLK_LCD                4
+#define LPC32XX_CLK_MAC                5

Quoted 15 lines · On Friday 20 November 2015 03:05:03 Vladimir Zapolskiy wrote:

include/dt-bindings/clock/lpc32xx-clock.h new file +15 -0

+
+/* LPC32XX System Control Block clocks */
+#define LPC32XX_CLK_RTC                0
+#define LPC32XX_CLK_DMA                1
+#define LPC32XX_CLK_MLC                2
+#define LPC32XX_CLK_SLC                3
+#define LPC32XX_CLK_LCD                4
+#define LPC32XX_CLK_MAC                5
+#define LPC32XX_CLK_SD         6
+#define LPC32XX_CLK_DDRAM      7
+#define LPC32XX_CLK_SSP0       8
+#define LPC32XX_CLK_SSP1       9
+#define LPC32XX_CLK_UART3      10
+#define LPC32XX_CLK_UART4      11
+#define LPC32XX_CLK_UART5      12

include/dt-bindings/clock/lpc32xx-clock.h line 31

Quoted 4 lines

include/dt-bindings/clock/lpc32xx-clock.h new file +3 -0

+#define LPC32XX_CLK_UART6      13
+#define LPC32XX_CLK_IRDA       14
+#define LPC32XX_CLK_I2C1       15

Any chance we can avoid the include file? This is going to make it really hard to merge everything in one merge window with the dependencies between the driver, the bindings and the platform code.

If there is a way to describe the clocks based on numbers from the data sheet instead of making up your own, that makes life much easier for us.

Quoted 16 lines · Arnd Bergmann

Quoted 15 lines · On Friday 20 November 2015 03:05:03 Vladimir Zapolskiy wrote:

include/dt-bindings/clock/lpc32xx-clock.h new file +15 -0

+
+/* LPC32XX System Control Block clocks */
+#define LPC32XX_CLK_RTC                0
+#define LPC32XX_CLK_DMA                1
+#define LPC32XX_CLK_MLC                2
+#define LPC32XX_CLK_SLC                3
+#define LPC32XX_CLK_LCD                4
+#define LPC32XX_CLK_MAC                5
+#define LPC32XX_CLK_SD         6
+#define LPC32XX_CLK_DDRAM      7
+#define LPC32XX_CLK_SSP0       8
+#define LPC32XX_CLK_SSP1       9
+#define LPC32XX_CLK_UART3      10
+#define LPC32XX_CLK_UART4      11
+#define LPC32XX_CLK_UART5      12

Quoted 8 lines

include/dt-bindings/clock/lpc32xx-clock.h line 31

Quoted 4 lines

include/dt-bindings/clock/lpc32xx-clock.h new file +3 -0

+#define LPC32XX_CLK_UART6      13
+#define LPC32XX_CLK_IRDA       14
+#define LPC32XX_CLK_I2C1       15

Any chance we can avoid the include file? This is going to make it really hard to merge everything in one merge window with the dependencies between the driver, the bindings and the platform code.

I see only one option to avoid this commit, namely squash it with the CCF driver and merge it before making changes in DTS.

However I suppose ARM trees won't be synced on clk tree, so probably it won't simplify maintainer's work.

Quoted 3 lines

If there is a way to describe the clocks based on numbers from the data sheet instead of making up your own, that makes life much easier for us.

There are no any clock numbers in the datasheet, unfortunately.

Quoted 26 lines · On Friday 20 November 2015 19:58:06 Vladimir Zapolskiy wrote:

Quoted 16 lines · Arnd Bergmann

Quoted 15 lines · On Friday 20 November 2015 03:05:03 Vladimir Zapolskiy wrote:

include/dt-bindings/clock/lpc32xx-clock.h new file +15 -0

+
+/* LPC32XX System Control Block clocks */
+#define LPC32XX_CLK_RTC                0
+#define LPC32XX_CLK_DMA                1
+#define LPC32XX_CLK_MLC                2
+#define LPC32XX_CLK_SLC                3
+#define LPC32XX_CLK_LCD                4
+#define LPC32XX_CLK_MAC                5
+#define LPC32XX_CLK_SD         6
+#define LPC32XX_CLK_DDRAM      7
+#define LPC32XX_CLK_SSP0       8
+#define LPC32XX_CLK_SSP1       9
+#define LPC32XX_CLK_UART3      10
+#define LPC32XX_CLK_UART4      11
+#define LPC32XX_CLK_UART5      12

Quoted 8 lines

include/dt-bindings/clock/lpc32xx-clock.h line 31

Quoted 4 lines

include/dt-bindings/clock/lpc32xx-clock.h new file +3 -0

+#define LPC32XX_CLK_UART6      13
+#define LPC32XX_CLK_IRDA       14
+#define LPC32XX_CLK_I2C1       15

Any chance we can avoid the include file? This is going to make it really hard to merge everything in one merge window with the dependencies between the driver, the bindings and the platform code.

Quoted 5 lines

I see only one option to avoid this commit, namely squash it with the CCF driver and merge it before making changes in DTS.

However I suppose ARM trees won't be synced on clk tree, so probably it won't simplify maintainer's work.

I think the best way for this would then be to have one git branch that contains the binding header, and base the other patches on top of that branch, for both the changes going into the clk git and arm-soc.

That way, both have the commit we need, but we don't get duplicate commits when they are both merged into mainline.

Quoted 5 lines

Quoted 3 lines

If there is a way to describe the clocks based on numbers from the data sheet instead of making up your own, that makes life much easier for us.

There are no any clock numbers in the datasheet, unfortunately.

I was thinking that maybe there would be a logical numbering based on the register layout, e.g. a tuple of register index and bit position, but it seems that the mmio area is too irregular to make that easy.

The change adds COMMON_CLK_NXP configuration symbol and enables it for NXP LPC18XX architecture, this is needed to reuse drivers/clk/nxp folder for NXP common clock framework drivers other than LPC18XX one.

Signed-off-by Vladimir Zapolskiy <[email protected]>

Cc Joachim Eastwood <[email protected]>

---
 arch/arm/Kconfig     | 1 +
 drivers/clk/Kconfig  | 5 +++++
 drivers/clk/Makefile | 2 +-
 3 files changed, 7 insertions(+), 1 deletion(-)

arch/arm/Kconfig +0 -0

index 0365cbb..c318277 100644

arch/arm/Kconfig +0 -0

        select PINCTRL
        help
          Support for NXP's LPC18xx Cortex-M3 and LPC43xx Cortex-M4

drivers/clk/Kconfig +5 -0

index c3e3a02..7fc1eb9 100644
@@ -175,6 +175,11 @@ config COMMON_CLK_PWM
          Adapter driver so that any PWM output can be (mis)used as clock signal
          at 50% duty cycle.
 
+config COMMON_CLK_NXP
+       def_bool COMMON_CLK && ARCH_LPC18XX
+       ---help---
+         Support for clock providers on NXP platforms.
+
 config COMMON_CLK_PXA
        def_bool COMMON_CLK && ARCH_PXA
        ---help---

drivers/clk/Makefile +0 -0

index 820714c..15603c1 100644

drivers/clk/nxp/clk-lpc32xx.c new file +9 -0

+
+struct clk_proto_t {
+       const char *name;
+       const u8 parents[LPC32XX_CLK_PARENTS_MAX];
+       u8 num_parents;
+       unsigned long flags;
+};
+
+#define CLK_PREFIX(LITERAL)            LPC32XX_CLK_ ## LITERAL

Quoted 9 lines · On Friday 20 November 2015 03:05:09 Vladimir Zapolskiy wrote:

drivers/clk/nxp/clk-lpc32xx.c new file +9 -0

+
+struct clk_proto_t {
+       const char *name;
+       const u8 parents[LPC32XX_CLK_PARENTS_MAX];
+       u8 num_parents;
+       unsigned long flags;
+};
+
+#define CLK_PREFIX(LITERAL)            LPC32XX_CLK_ ## LITERAL

drivers/clk/nxp/clk-lpc32xx.c line 202

Quoted 10 lines

drivers/clk/nxp/clk-lpc32xx.c new file +10 -0

+#define NUMARGS(...)   (sizeof((int[]){__VA_ARGS__})/sizeof(int))
+
+#define LPC32XX_CLK_DEFINE(_idx, _name, _flags, ...)           \
+       [CLK_PREFIX(_idx)] = {                                  \
+               .name = #_name,                                 \
+               .flags = _flags,                                \
+               .parents = { __VA_ARGS__ },                     \
+               .num_parents = NUMARGS(__VA_ARGS__),            \
+        }
+

Try to not outsmart yourself with the macros. It's better to avoid string concatenation so it's possible to grep for uses of some constant.

I would probably not use a macro at all here and just open-code the entire table. If you ensure that '0' is not a valid parent, then you can leave out the .num_parents field and just look for the zero-termination.

Quoted 25 lines · Arnd Bergmann

Quoted 9 lines · On Friday 20 November 2015 03:05:09 Vladimir Zapolskiy wrote:

drivers/clk/nxp/clk-lpc32xx.c new file +9 -0

+
+struct clk_proto_t {
+       const char *name;
+       const u8 parents[LPC32XX_CLK_PARENTS_MAX];
+       u8 num_parents;
+       unsigned long flags;
+};
+
+#define CLK_PREFIX(LITERAL)            LPC32XX_CLK_ ## LITERAL

drivers/clk/nxp/clk-lpc32xx.c line 202

Quoted 10 lines

drivers/clk/nxp/clk-lpc32xx.c new file +10 -0

+#define NUMARGS(...)   (sizeof((int[]){__VA_ARGS__})/sizeof(int))
+
+#define LPC32XX_CLK_DEFINE(_idx, _name, _flags, ...)           \
+       [CLK_PREFIX(_idx)] = {                                  \
+               .name = #_name,                                 \
+               .flags = _flags,                                \
+               .parents = { __VA_ARGS__ },                     \
+               .num_parents = NUMARGS(__VA_ARGS__),            \
+        }
+

Try to not outsmart yourself with the macros. It's better to avoid string concatenation so it's possible to grep for uses of some constant.

Quoted 4 lines

I would probably not use a macro at all here and just open-code the entire table. If you ensure that '0' is not a valid parent, then you can leave out the .num_parents field and just look for the zero-termination.

Macros are here for simplicity, code size reduction and to avoid some stupid mistakes like different number of .parents and .num_parents.

I believe macro unwrapping in this code will add another 1000 LoC and will result in quite unreadable and less maintainable code.

Quoted 31 lines · On Friday 20 November 2015 20:07:46 Vladimir Zapolskiy wrote:

Quoted 25 lines · Arnd Bergmann

Quoted 9 lines · On Friday 20 November 2015 03:05:09 Vladimir Zapolskiy wrote:

drivers/clk/nxp/clk-lpc32xx.c new file +9 -0

+
+struct clk_proto_t {
+       const char *name;
+       const u8 parents[LPC32XX_CLK_PARENTS_MAX];
+       u8 num_parents;
+       unsigned long flags;
+};
+
+#define CLK_PREFIX(LITERAL)            LPC32XX_CLK_ ## LITERAL

drivers/clk/nxp/clk-lpc32xx.c line 202

Quoted 10 lines

drivers/clk/nxp/clk-lpc32xx.c new file +10 -0

+#define NUMARGS(...)   (sizeof((int[]){__VA_ARGS__})/sizeof(int))
+
+#define LPC32XX_CLK_DEFINE(_idx, _name, _flags, ...)           \
+       [CLK_PREFIX(_idx)] = {                                  \
+               .name = #_name,                                 \
+               .flags = _flags,                                \
+               .parents = { __VA_ARGS__ },                     \
+               .num_parents = NUMARGS(__VA_ARGS__),            \
+        }
+

Try to not outsmart yourself with the macros. It's better to avoid string concatenation so it's possible to grep for uses of some constant.

Quoted 4 lines

I would probably not use a macro at all here and just open-code the entire table. If you ensure that '0' is not a valid parent, then you can leave out the .num_parents field and just look for the zero-termination.

Quoted 5 lines

Macros are here for simplicity, code size reduction and to avoid some stupid mistakes like different number of .parents and .num_parents.

I believe macro unwrapping in this code will add another 1000 LoC and will result in quite unreadable and less maintainable code.

I mean specifically the macro above:

static const struct clk_proto_t clk_proto[LPC32XX_CLK_CCF_MAX] __initconst = {

drivers/clk/nxp/clk-lpc32xx.c new file +9 -0

+       LPC32XX_CLK_DEFINE(XTAL, xtal, 0x0),
+       LPC32XX_CLK_DEFINE(XTAL_32K, xtal_32k, 0x0),
+
+       LPC32XX_CLK_DEFINE(RTC, rtc, 0x0, LPC32XX_CLK_XTAL_32K),
+       LPC32XX_CLK_DEFINE(OSC, osc, CLK_IGNORE_UNUSED, LPC32XX_CLK_XTAL),
+       LPC32XX_CLK_DEFINE(SYS, sys, CLK_IGNORE_UNUSED,
+               LPC32XX_CLK_OSC, LPC32XX_CLK_PLL397X),
+       LPC32XX_CLK_DEFINE(PLL397X, pll_397x, CLK_IGNORE_UNUSED,
+               LPC32XX_CLK_RTC),

can become

static const struct clk_proto_t clk_proto[] __initconst = {
	[LPC32XX_CLK_XTAL]	= { "xtal" },
	[LPC32XX_CLK_XTAL_32K]	= { "xtal_32k" },
	[LPC32XX_CLK_RTC]	= { "rtc",
			.parents = { LPC32XX_CLK_XTAL_32K, 0 } },

	[LPC32XX_CLK_OSC]	= { "osc", CLK_IGNORE_UNUSED,
			.parents = { LPC32XX_CLK_XTAL, 0 } },
	[LPC32XX_CLK_SYS]	= { "sys", CLK_IGNORE_UNUSED,
			.parents = { LPC32XX_CLK_OSC, LPC32XX_CLK_PLL397X, 0) },
	[LPC32XX_CLK_PLL397X]	= { "pll_397x", CLK_IGNORE_UNUSED,
			.parents = { LPC32XX_CLK_RTC, 0 },

Not harder to read at all, not really longer, but easier to grep for.

This changeset adds common clock framework driver for NXP LPC32xx boards.

Vladimir Zapolskiy (11):
dt-bindings: clock: add description of LPC32xx clock controller
dt-bindings: clock: add description of LPC32xx USB clock controller
dt-bindings: clock: add NXP LPC32xx clock list for consumers
arm: dts: lpc32xx: add device nodes for external oscillators

arm: dts: lpc32xx: add clock controller device node
arm: dts: lpc32xx: add clock properties to device nodes
arm: dts: lpc32xx: add USB clock controller
clk: lpc18xx: add NXP specific common clock framework selection
clk: lpc32xx: add common clock framework driver
arm: lpc32xx: switch to common clock framework
arm: dts: lpc32xx: remove clock frequency property from UART device
nodes

Quoted 3 lines · Vladimir Zapolskiy

This changeset adds common clock framework driver for NXP LPC32xx boards.

Hi Vladimir,

Just to let you know that we did some testing using this changeset; I installed patches 4 to 7, 10 & 11 on top of 4.5-rc1.

I was able to run our software (SIC1 interrupt sources disable) without any problem a custom LPC32xx board.

We did not get any problem with the clock signals we are using.

Quoted 5 lines

Vladimir Zapolskiy (11):
dt-bindings: clock: add description of LPC32xx clock controller
dt-bindings: clock: add description of LPC32xx USB clock controller
dt-bindings: clock: add NXP LPC32xx clock list for consumers
arm: dts: lpc32xx: add device nodes for external oscillators

Quoted 8 lines

arm: dts: lpc32xx: add clock controller device node
arm: dts: lpc32xx: add clock properties to device nodes
arm: dts: lpc32xx: add USB clock controller
clk: lpc18xx: add NXP specific common clock framework selection
clk: lpc32xx: add common clock framework driver
arm: lpc32xx: switch to common clock framework
arm: dts: lpc32xx: remove clock frequency property from UART device
nodes

Hi Sylvain,

Quoted 8 lines · Sylvain Lemieux

Quoted 3 lines · Vladimir Zapolskiy

This changeset adds common clock framework driver for NXP LPC32xx boards.

Just to let you know that we did some testing using this changeset; I installed patches 4 to 7, 10 & 11 on top of 4.5-rc1.

Quoted 2 lines

I was able to run our software (SIC1 interrupt sources disable) without any problem a custom LPC32xx board.

this is a known issue, and I hope it will be fixed in upstream in v4.6, the required changes in irqchip and gpiochip drivers were sent for review some time ago, a bit more work is needed to

Also 2-3 more clean-up changes in USB host and gadget drivers are done locally (no more need to configure clocks out of the common clock framework), but not yet submitted, will do it tomorrow.

Quoted 2 lines

We did not get any problem with the clock signals we are using.

Thank you for testing, I'm going to add your Tested-by tag (if you don't mind) and submit a PR to ARM maintainers shortly.

Quoted 5 lines

Quoted 5 lines

Vladimir Zapolskiy (11):
dt-bindings: clock: add description of LPC32xx clock controller
dt-bindings: clock: add description of LPC32xx USB clock controller
dt-bindings: clock: add NXP LPC32xx clock list for consumers
arm: dts: lpc32xx: add device nodes for external oscillators

Quoted 10 lines

Quoted 8 lines

arm: dts: lpc32xx: add clock controller device node
arm: dts: lpc32xx: add clock properties to device nodes
arm: dts: lpc32xx: add USB clock controller
clk: lpc18xx: add NXP specific common clock framework selection
clk: lpc32xx: add common clock framework driver
arm: lpc32xx: switch to common clock framework
arm: dts: lpc32xx: remove clock frequency property from UART device
nodes

With best wishes,
Vladimir