Skip to contentSkip to conversation

intel: Add a new driver for a new clock controller IP

PATCH Superseded Rahul Tanwar 3 participants 9 messages Feb 19, 2020 – Mar 4, 2020

Subsystems (2)

v5 of 8 Superseded Latest is v8 →

lore
Share & tools
lore
#0 lore raw

v5 cover intel: Add a new driver for a new clock controller IP

Hi,

This series adds clock driver for Clock Generation Unit(CGU) of Lightning Mountain(LGM) SoC.

Patch 1 adds bindings document & include file for CGU. Patch 2 adds common clock framework based clock driver for CGU.

These patches are baselined upon Linux 5.6-rc1 at below Git link:
git git://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git

v5:
- Address review concerns - mainly below mentioned. (Stephen Boyd)
- Improve commit message, add COMPILE_TEST in KConfig dependency.
- Remove unused header include files, drop unnecessary casts.
- Switch to using readl_poll_timeout() instead of implementing timeout routine.
- Avoid using small functions which are called just once. Inline them or remove them.
- const static --> static const
- Fix coding style/convention related review concerns.
- Use __iomem for all IO addresses variables.
- Consolidate clk_enable & clk_disable ops into a common clk_enable_disable routine to avoid redundant code.
- Remove unnecessary dev pointers for clk data structures.
- Redesign code to use new way of specifying clk_parents i.e. use clk_parent_data.fw_name instead of older parent_name strings.
- Switch from raw_spin_locks() to normal spin_locks() and realign locking.
- Drop __initconst, __init, __refdata.
- Reorder patch series - make dt-binding patch as first patch.
- Add pointer to include file in dt-bindings document.
- Remove CLK_IS_CRITICAL flag for clks for which IGNORE_UNUSED flag is enough. Add comments for clks which are marked as CRITICAL.
- Fix $id path in dt-bindings - drop bindings. (Rob Herring).
- Add Reviewed-by tag from Rob Herring. Thanks Rob.

v4:
- Add drivers/clk/x86/Kconfig file which got missed in v3 by mistake.

v3:
- Address review concerns:
Add Kconfig entry in x86 folder instead of modifying clk/Kconfig. (Andy Shevchenko) Fix coding style/convention related concerns. (Andy Shevchenko) Improve description, licensing info, rename node name correctly in dt bindings document & remove CLK_NR_CLKS from dt-bindings header file. (Stephen Boyd) Fix a build warning reported by kbuild test robot & Nathan Chancellor
- Add few new clocks & rename few existing clocks.
- Add more ops for ddiv & divider clk_ops.
- Fix few minor bugs.
- Use CLK_IS_CRITICAL flag for clocks which shall never be disabled.

v2:
- Move the driver to x86 folder.
- Remove syscon usage.
- Remove regmap based access. Use direct readl()/write() instead. Add spinlocks.
- Change all enum values to capitals.
- Rename all data structures & functions from intel_* to lgm_*.
- Remove multiple header files. Keep only one header file.
- Make probe fail when any of the clk/pll registration fails.
- Fix few bugs with clk_init_data assignement.
- Address review concerns for code quality/style/convention.

v1:
- Initial version.

Rahul Tanwar (1):
dt-bindings: clk: intel: Add bindings document & header file for CGU

rtanwar (1):
clk: intel: Add CGU clock driver for a new SoC

 .../devicetree/bindings/clock/intel,cgu-lgm.yaml   |  44 ++
 drivers/clk/Kconfig                                |   1 +
 drivers/clk/x86/Kconfig                            |   8 +
 drivers/clk/x86/Makefile                           |   1 +
 drivers/clk/x86/clk-cgu-pll.c                      | 156 +++++
 drivers/clk/x86/clk-cgu.c                          | 636 +++++++++++++++++++++
 drivers/clk/x86/clk-cgu.h                          | 335 +++++++++++
 drivers/clk/x86/clk-lgm.c                          | 492 ++++++++++++++++
 include/dt-bindings/clock/intel,lgm-clk.h          | 165 ++++++
 9 files changed, 1838 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/intel,cgu-lgm.yaml
 create mode 100644 drivers/clk/x86/Kconfig
 create mode 100644 drivers/clk/x86/clk-cgu-pll.c
 create mode 100644 drivers/clk/x86/clk-cgu.c
 create mode 100644 drivers/clk/x86/clk-cgu.h
 create mode 100644 drivers/clk/x86/clk-lgm.c
 create mode 100644 include/dt-bindings/clock/intel,lgm-clk.h
Signature · 1 line
2.11.0
#1 lore raw

PATCH v5 1/2 dt-bindings: clk: intel: Add bindings document & header file for CGU

Clock generation unit(CGU) is a clock controller IP of Intel's Lightning Mountain(LGM) SoC. Add DT bindings include file and document for CGU clock controller driver of LGM.

---
 .../devicetree/bindings/clock/intel,cgu-lgm.yaml   |  44 ++++++
 include/dt-bindings/clock/intel,lgm-clk.h          | 165 +++++++++++++++++++++
 2 files changed, 209 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/intel,cgu-lgm.yaml
 create mode 100644 include/dt-bindings/clock/intel,lgm-clk.h
Documentation/devicetree/bindings/clock/intel,cgu-lgm.yaml new file +44 -0
new file mode 100644
index 000000000000..6dc1414bfb7f
@@ -0,0 +1,44 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/intel,cgu-lgm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Intel Lightning Mountain SoC's Clock Controller(CGU) Binding
+
+maintainers:
+  - Rahul Tanwar <[email protected]>
+
+description: |
+  Lightning Mountain(LGM) SoC's Clock Generation Unit(CGU) driver provides
+  all means to access the CGU hardware module in order to generate a series
+  of clocks for the whole system and individual peripherals.
+
+  Please refer to include/dt-bindings/clock/intel,lgm-clk.h header file, it
+  defines all available clocks as macros. These macros can be used in device
+  tree sources.
+
+properties:
+  compatible:
+    const: intel,cgu-lgm
+
+  reg:
+    maxItems: 1
+
+  '#clock-cells':
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - '#clock-cells'
+
+examples:
+  - |
+    cgu: clock-controller@e0200000 {
+        compatible = "intel,cgu-lgm";
+        reg = <0xe0200000 0x33c>;
+        #clock-cells = <1>;
+    };
+
+...
include/dt-bindings/clock/intel,lgm-clk.h new file +165 -0

165 lines not on this page · raw message

Signature · 1 line
2.11.0
#2 lore raw

PATCH v5 2/2 clk: intel: Add CGU clock driver for a new SoC

In reply to Rahul Tanwar

From: rtanwar <[email protected]>

Clock Generation Unit(CGU) is a new clock controller IP of a forthcoming Intel network processor SoC named Lightning Mountain(LGM). It provides programming interfaces to control & configure all CPU & peripheral clocks. Add common clock framework based clock controller driver for CGU.

---
 drivers/clk/Kconfig           |   1 +
 drivers/clk/x86/Kconfig       |   8 +
 drivers/clk/x86/Makefile      |   1 +
 drivers/clk/x86/clk-cgu-pll.c | 156 +++++++++++
 drivers/clk/x86/clk-cgu.c     | 636 ++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/x86/clk-cgu.h     | 335 ++++++++++++++++++++++
 drivers/clk/x86/clk-lgm.c     | 492 ++++++++++++++++++++++++++++++++
 7 files changed, 1629 insertions(+)
 create mode 100644 drivers/clk/x86/Kconfig
 create mode 100644 drivers/clk/x86/clk-cgu-pll.c
 create mode 100644 drivers/clk/x86/clk-cgu.c
 create mode 100644 drivers/clk/x86/clk-cgu.h
 create mode 100644 drivers/clk/x86/clk-lgm.c
drivers/clk/Kconfig +1 -0
index bcb257baed06..43dab257e7aa 100644
@@ -360,6 +360,7 @@ source "drivers/clk/sunxi-ng/Kconfig"
 source "drivers/clk/tegra/Kconfig"
 source "drivers/clk/ti/Kconfig"
 source "drivers/clk/uniphier/Kconfig"
+source "drivers/clk/x86/Kconfig"
 source "drivers/clk/zynqmp/Kconfig"
 
 endmenu
drivers/clk/x86/Kconfig new file +8 -0
new file mode 100644
index 000000000000..2e2b9730541f
@@ -0,0 +1,8 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config CLK_LGM_CGU
+	depends on (OF && HAS_IOMEM) || COMPILE_TEST
+	select OF_EARLY_FLATTREE
+	bool "Clock driver for Lightning Mountain(LGM) platform"
+	help
+	  Clock Generation Unit(CGU) driver for Intel Lightning Mountain(LGM)
+	  network processor SoC.
drivers/clk/x86/Makefile +1 -0
index e3ec81e2a1c2..7c774ea7ddeb 100644
@@ -3,3 +3,4 @@ obj-$(CONFIG_PMC_ATOM)		+= clk-pmc-atom.o
 obj-$(CONFIG_X86_AMD_PLATFORM_DEVICE)	+= clk-st.o
 clk-x86-lpss-objs		:= clk-lpt.o
 obj-$(CONFIG_X86_INTEL_LPSS)	+= clk-x86-lpss.o
+obj-$(CONFIG_CLK_LGM_CGU)	+= clk-cgu.o clk-cgu-pll.o clk-lgm.o
drivers/clk/x86/clk-cgu-pll.c new file +156 -0

156 lines not on this page · raw message

drivers/clk/x86/clk-cgu.c new file +636 -0

636 lines not on this page · raw message

drivers/clk/x86/clk-cgu.h new file +335 -0

335 lines not on this page · raw message

drivers/clk/x86/clk-lgm.c new file +492 -0

492 lines not on this page · raw message

Signature · 1 line
2.11.0
#3 lore raw

Re: [PATCH v5 2/2] clk: intel: Add CGU clock driver for a new SoC

Quoted 28 lines · Rahul Tanwar

From: rtanwar <[email protected]>

Clock Generation Unit(CGU) is a new clock controller IP of a forthcoming Intel network processor SoC named Lightning Mountain(LGM). It provides programming interfaces to control & configure all CPU & peripheral clocks. Add common clock framework based clock controller driver for CGU.

---
 drivers/clk/Kconfig           |   1 +
 drivers/clk/x86/Kconfig       |   8 +
 drivers/clk/x86/Makefile      |   1 +
 drivers/clk/x86/clk-cgu-pll.c | 156 +++++++++++
 drivers/clk/x86/clk-cgu.c     | 636 ++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/x86/clk-cgu.h     | 335 ++++++++++++++++++++++
 drivers/clk/x86/clk-lgm.c     | 492 ++++++++++++++++++++++++++++++++
 7 files changed, 1629 insertions(+)
 create mode 100644 drivers/clk/x86/Kconfig
 create mode 100644 drivers/clk/x86/clk-cgu-pll.c
 create mode 100644 drivers/clk/x86/clk-cgu.c
 create mode 100644 drivers/clk/x86/clk-cgu.h
 create mode 100644 drivers/clk/x86/clk-lgm.c
drivers/clk/Kconfig +0 -0
index bcb257baed06..43dab257e7aa 100644

drivers/clk/Kconfig source "drivers/clk/sunxi-ng/Kconfig" · line 366

Quoted 7 lines
drivers/clk/Kconfig +1 -0
@@ -360,6 +360,7 @@ source "drivers/clk/sunxi-ng/Kconfig"
 source "drivers/clk/tegra/Kconfig"
 source "drivers/clk/ti/Kconfig"
 source "drivers/clk/uniphier/Kconfig"
+source "drivers/clk/x86/Kconfig"
 source "drivers/clk/zynqmp/Kconfig"
 
 endmenu

Hi,

drivers/clk/x86/Kconfig line 3

Quoted 9 lines
drivers/clk/x86/Kconfig new file +3 -0
new file mode 100644
index 000000000000..2e2b9730541f
@@ -0,0 +1,8 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config CLK_LGM_CGU
+	depends on (OF && HAS_IOMEM) || COMPILE_TEST

This "depends on" looks problematic to me. I guess we shall see when all the build bots get to it.

drivers/clk/x86/Kconfig line 4

Quoted 1 line
drivers/clk/x86/Kconfig new file +1 -0
+	select OF_EARLY_FLATTREE

If OF is not set and HAS_IOMEM is not set, but COMPILE_TEST is set, I expect that this should not be attempting to select OF_EARLY_FLATTREE.

Have you tried such a config combination?

drivers/clk/x86/Kconfig line 8

Quoted 4 lines
drivers/clk/x86/Kconfig new file +4 -0
+	bool "Clock driver for Lightning Mountain(LGM) platform"
+	help
+	  Clock Generation Unit(CGU) driver for Intel Lightning Mountain(LGM)
+	  network processor SoC.

thanks.

Signature · 1 line
~Randy
#4 lore raw

Re: [PATCH v5 2/2] clk: intel: Add CGU clock driver for a new SoC

Hi Randy,

Quoted 38 lines · Randy Dunlap
Quoted 28 lines · Rahul Tanwar

From: rtanwar <[email protected]>

Clock Generation Unit(CGU) is a new clock controller IP of a forthcoming Intel network processor SoC named Lightning Mountain(LGM). It provides programming interfaces to control & configure all CPU & peripheral clocks. Add common clock framework based clock controller driver for CGU.

---
 drivers/clk/Kconfig           |   1 +
 drivers/clk/x86/Kconfig       |   8 +
 drivers/clk/x86/Makefile      |   1 +
 drivers/clk/x86/clk-cgu-pll.c | 156 +++++++++++
 drivers/clk/x86/clk-cgu.c     | 636 ++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/x86/clk-cgu.h     | 335 ++++++++++++++++++++++
 drivers/clk/x86/clk-lgm.c     | 492 ++++++++++++++++++++++++++++++++
 7 files changed, 1629 insertions(+)
 create mode 100644 drivers/clk/x86/Kconfig
 create mode 100644 drivers/clk/x86/clk-cgu-pll.c
 create mode 100644 drivers/clk/x86/clk-cgu.c
 create mode 100644 drivers/clk/x86/clk-cgu.h
 create mode 100644 drivers/clk/x86/clk-lgm.c
drivers/clk/Kconfig +0 -0
index bcb257baed06..43dab257e7aa 100644

drivers/clk/Kconfig source "drivers/clk/sunxi-ng/Kconfig" · line 366

Quoted 7 lines
drivers/clk/Kconfig +1 -0
@@ -360,6 +360,7 @@ source "drivers/clk/sunxi-ng/Kconfig"
 source "drivers/clk/tegra/Kconfig"
 source "drivers/clk/ti/Kconfig"
 source "drivers/clk/uniphier/Kconfig"
+source "drivers/clk/x86/Kconfig"
 source "drivers/clk/zynqmp/Kconfig"
 
 endmenu

Hi,

Quoted 11 lines

drivers/clk/x86/Kconfig line 3

Quoted 9 lines
drivers/clk/x86/Kconfig new file +3 -0
new file mode 100644
index 000000000000..2e2b9730541f
@@ -0,0 +1,8 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config CLK_LGM_CGU
+	depends on (OF && HAS_IOMEM) || COMPILE_TEST

This "depends on" looks problematic to me. I guess we shall see when all the build bots get to it.

At the moment, i am not able to figure out possible problems in this..

Quoted 5 lines

drivers/clk/x86/Kconfig line 4

Quoted 1 line
drivers/clk/x86/Kconfig new file +1 -0
+	select OF_EARLY_FLATTREE

If OF is not set and HAS_IOMEM is not set, but COMPILE_TEST is set, I expect that this should not be attempting to select OF_EARLY_FLATTREE.

Have you tried such a config combination?

Agree, that would be a problem. I will change it to

select OF_EARLY_FLATTREE if OF

Thanks.

Regards,
Rahul

#5 lore raw

Re: [PATCH v5 2/2] clk: intel: Add CGU clock driver for a new SoC

Quoted 9 lines · Rahul
Quoted 6 lines · Randy Dunlap

drivers/clk/x86/Kconfig line 3

Quoted 2 lines · Rahul Tanwar
drivers/clk/x86/Kconfig new file +2 -0
+config CLK_LGM_CGU
+	depends on (OF && HAS_IOMEM) || COMPILE_TEST

This "depends on" looks problematic to me. I guess we shall see when all the build bots get to it.

At the moment, i am not able to figure out possible problems in this..

COMPILE_TEST should be accompanied by non-generic dependency. There is none.

So, I quite agree with Randy.

Quoted 9 lines
Quoted 5 lines

drivers/clk/x86/Kconfig line 4

Quoted 1 line
drivers/clk/x86/Kconfig new file +1 -0
+	select OF_EARLY_FLATTREE

If OF is not set and HAS_IOMEM is not set, but COMPILE_TEST is set, I expect that this should not be attempting to select OF_EARLY_FLATTREE.

Have you tried such a config combination?

Agree, that would be a problem. I will change it to

select OF_EARLY_FLATTREE if OF

Nope, I think this is wrong work around.
See above.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#6 lore raw

Re: [PATCH v5 2/2] clk: intel: Add CGU clock driver for a new SoC

Quoted 8 lines · Andy Shevchenko
Quoted 7 lines · Rahul
Quoted 6 lines · Randy Dunlap

drivers/clk/x86/Kconfig line 3

Quoted 2 lines · Rahul Tanwar
drivers/clk/x86/Kconfig new file +2 -0
+config CLK_LGM_CGU
+	depends on (OF && HAS_IOMEM) || COMPILE_TEST

This "depends on" looks problematic to me. I guess we shall see when all the build bots get to it.

Quoted 5 lines
Quoted 1 line

At the moment, i am not able to figure out possible problems in this..

COMPILE_TEST should be accompanied by non-generic dependency. There is none.

So, I quite agree with Randy.

I see COMPILE_TEST is mostly ORed with ARCH_xx. How about below?

depends on OF && HAS_IOMEM && (CONFIG_X86 || COMPILE_TEST)

Quoted 10 lines
Quoted 8 lines
Quoted 5 lines

drivers/clk/x86/Kconfig line 4

Quoted 1 line
drivers/clk/x86/Kconfig new file +1 -0
+	select OF_EARLY_FLATTREE

If OF is not set and HAS_IOMEM is not set, but COMPILE_TEST is set, I expect that this should not be attempting to select OF_EARLY_FLATTREE.

Have you tried such a config combination?

Agree, that would be a problem. I will change it to

select OF_EARLY_FLATTREE if OF

Nope, I think this is wrong work around.
See above.

With above proposed change, i can simply switch to select OF_EARLY_FLATTREE since all dependencies are already in place..

Thanks.

Regards,
Rahul

#7 lore raw

Re: [PATCH v5 2/2] clk: intel: Add CGU clock driver for a new SoC

Quoted 10 lines · Rahul
Quoted 9 lines · Andy Shevchenko
Quoted 8 lines · Rahul
Quoted 6 lines · Randy Dunlap

drivers/clk/x86/Kconfig line 3

Quoted 2 lines · Rahul Tanwar
drivers/clk/x86/Kconfig new file +2 -0
+config CLK_LGM_CGU
+	depends on (OF && HAS_IOMEM) || COMPILE_TEST

This "depends on" looks problematic to me. I guess we shall see when all the build bots get to it.

At the moment, i am not able to figure out possible problems in this..

Quoted 8 lines
Quoted 4 lines

COMPILE_TEST should be accompanied by non-generic dependency. There is none.

So, I quite agree with Randy.

I see COMPILE_TEST is mostly ORed with ARCH_xx. How about below?

depends on OF && HAS_IOMEM && (CONFIG_X86 || COMPILE_TEST)

How about to leave logical parts separately?
How is OF related to architecture?

On top of that, is this code only for x86 for sure?

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines

drivers/clk/x86/Kconfig line 4

Quoted 1 line
drivers/clk/x86/Kconfig new file +1 -0
+	select OF_EARLY_FLATTREE

If OF is not set and HAS_IOMEM is not set, but COMPILE_TEST is set, I expect that this should not be attempting to select OF_EARLY_FLATTREE.

Have you tried such a config combination?

Agree, that would be a problem. I will change it to

Quoted 7 lines
Quoted 3 lines
Quoted 1 line

select OF_EARLY_FLATTREE if OF

Nope, I think this is wrong work around.
See above.

With above proposed change, i can simply switch to select OF_EARLY_FLATTREE since all dependencies are already in place..

Right.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#8 lore raw

Re: [PATCH v5 2/2] clk: intel: Add CGU clock driver for a new SoC

Quoted 14 lines · Andy Shevchenko
Quoted 13 lines · Rahul
Quoted 12 lines · Andy Shevchenko
Quoted 8 lines · Rahul
Quoted 6 lines · Randy Dunlap

drivers/clk/x86/Kconfig line 3

Quoted 2 lines · Rahul Tanwar
drivers/clk/x86/Kconfig new file +2 -0
+config CLK_LGM_CGU
+	depends on (OF && HAS_IOMEM) || COMPILE_TEST

This "depends on" looks problematic to me. I guess we shall see when all the build bots get to it.

At the moment, i am not able to figure out possible problems in this..

COMPILE_TEST should be accompanied by non-generic dependency. There is none.

Quoted 6 lines
Quoted 4 lines
Quoted 1 line

So, I quite agree with Randy.

I see COMPILE_TEST is mostly ORed with ARCH_xx. How about below?

depends on OF && HAS_IOMEM && (CONFIG_X86 || COMPILE_TEST)

How about to leave logical parts separately?
How is OF related to architecture?

OF is not related to architecture. Driver depends on OF. In the past, many build/linker issues were reported due to OF & HAS_IOMEM dependencies in the code. Please see [1] & [2]. So to be safe this time, i am adding these dependencies here.

Quoted 2 lines

On top of that, is this code only for x86 for sure?

Yes, this is a totally new IP for x86 based SoC. No plans of using same IP/driver for any other arch based SoCs.

[1] https://lkml.org/lkml/2019/12/3/613
[2] https://lkml.org/lkml/2019/12/11/1733