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

From: rtanwar <[email protected]>

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

 drivers/clk/intel/Kconfig       |  13 +
 drivers/clk/intel/Makefile      |   4 +

drivers/clk/intel/clk-cgu-pll.c new file +5 -0

+/*
+ * Calculate formula:
+ * rate = (prate * mult + (prate * frac) / frac_div) / div
+ */
+static unsigned long

drivers/clk/intel/clk-cgu-pll.c new file +16 -0

+			const struct intel_pll_clk_data *list)
+{
+	struct clk_init_data init;
+	struct intel_clk_pll *pll;
+	struct device *dev = ctx->dev;
+	struct clk_hw *hw;
+	int ret;
+
+	init.ops = &intel_lgm_pll_ops;
+	init.name = list->name;
+	init.parent_names = list->parent_names;
+	init.num_parents = list->num_parents;
+
+	pll = devm_kzalloc(dev, sizeof(*pll), GFP_KERNEL);
+	if (!pll)
+		return ERR_PTR(-ENOMEM);

+/*
+ * Copyright (C) 2018 Intel Corporation.
+ * Zhu YiXin <[email protected]>

drivers/clk/intel/clk-lgm.c new file +10 -0

+	{ .val = 1, .div = 2 },
+	{ .val = 2, .div = 3 },
+	{ .val = 3, .div = 4 },
+	{ .val = 4, .div = 5 },
+	{ .val = 5, .div = 6 },
+	{ .val = 6, .div = 8 },
+	{ .val = 7, .div = 10 },
+	{ .val = 8, .div = 12 },
+	{ .val = 9, .div = 16 },
+	{ .val = 10, .div = 20 },

+}

Quoted 8 lines · Rahul Tanwar

From: rtanwar <[email protected]>

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

  drivers/clk/intel/Kconfig       |  13 +
  drivers/clk/intel/Makefile      |   4 +

Any plans what to do with existing x86 folder there?

drivers/clk/intel/Kconfig line 6

Quoted 8 lines

+++ b/drivers/clk/intel/Kconfig

drivers/clk/intel/Kconfig new file +6 -0

@@ -0,0 +1,13 @@
+# SPDX-License-Identifier: GPL-2.0
+config INTEL_LGM_CGU_CLK
+	depends on COMMON_CLK
+	select MFD_SYSCON
+	select OF_EARLY_FLATTREE
+	bool "Intel Clock Genration Unit support"

Is it for X86? Don't you need a dependency?

Quoted 10 lines

drivers/clk/intel/clk-cgu-pll.c new file +10 -0

+/*
+ * Calculate formula:
+ * rate = (prate * mult + (prate * frac) / frac_div) / div
+ */
+static unsigned long
+intel_pll_calc_rate(unsigned long prate, unsigned int mult,
+		    unsigned int div, unsigned int frac, unsigned int frac_div)
+{
+	u64 crate, frate, rate64;
+

drivers/clk/intel/clk-cgu-pll.c line 35

Quoted 4 lines

drivers/clk/intel/clk-cgu-pll.c new file +4 -0

+	rate64 = prate;
+	crate = rate64 * mult;
+
+	if (frac) {

This seems unnecessary.
I think you would like to check for frac_div instead?
Though I would rather to use frac = 0, frac_div = 1 and drop this conditional completely.

drivers/clk/intel/clk-cgu-pll.c

Quoted 10 lines

drivers/clk/intel/clk-cgu-pll.c new file +10 -0

+		frate = rate64 * frac;
+		do_div(frate, frac_div);
+		crate += frate;
+	}
+	do_div(crate, div);
+
+	return (unsigned long)crate;
+}
+static struct clk_hw
+*intel_clk_register_pll(struct intel_clk_provider *ctx,

* is part of type.

Quoted 18 lines

drivers/clk/intel/clk-cgu-pll.c new file +18 -0

+			const struct intel_pll_clk_data *list)
+{
+	struct clk_init_data init;
+	struct intel_clk_pll *pll;
+	struct device *dev = ctx->dev;
+	struct clk_hw *hw;
+	int ret;
+
+	init.ops = &intel_lgm_pll_ops;
+	init.name = list->name;
+	init.parent_names = list->parent_names;
+	init.num_parents = list->num_parents;
+
+	pll = devm_kzalloc(dev, sizeof(*pll), GFP_KERNEL);
+	if (!pll)
+		return ERR_PTR(-ENOMEM);
+
+	pll->map = ctx->map;

drivers/clk/intel/clk-cgu-pll.c line 135

Quoted 7 lines

drivers/clk/intel/clk-cgu-pll.c new file +7 -0

+	pll->dev = ctx->dev;
+	pll->reg = list->reg;
+	pll->flags = list->flags;
+	pll->type = list->type;
+	pll->hw.init = &init;
+
+	hw = &pll->hw;

Seems redundant temporary variable.

drivers/clk/intel/clk-cgu-pll.c line 145

Quoted 9 lines

drivers/clk/intel/clk-cgu-pll.c new file +9 -0

+	ret = clk_hw_register(dev, hw);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return hw;
+}
+void intel_clk_register_plls(struct intel_clk_provider *ctx,
+			     const struct intel_pll_clk_data *list,
+				unsigned int nr_clk)

Indentation issues.

drivers/clk/intel/clk-cgu-pll.c line 154

Quoted 9 lines

drivers/clk/intel/clk-cgu-pll.c new file +9 -0

+{
+	struct clk_hw *hw;
+	int i;
+
+	for (i = 0; i < nr_clk; i++, list++) {
+		hw = intel_clk_register_pll(ctx, list);
+		if (IS_ERR(hw)) {
+			dev_err(ctx->dev, "failed to register pll: %s\n",
+				list->name);

Is it fatal or not?

drivers/clk/intel/clk-cgu-pll.c line 159

Quoted 5 lines

drivers/clk/intel/clk-cgu-pll.c new file +5 -0

+			continue;
+		}
+
+		intel_clk_add_lookup(ctx, hw, list->id);
+	}

No error to return? Are all PLLs optional?

Review

Quoted 2 lines

(quoted diff) +2 -0

+}
+#endif				/* __INTEL_CLK_PLL_H */

One TAB is enough.

Quoted 3 lines

+/*
+ * Copyright (C) 2018 Intel Corporation.
+ * Zhu YiXin <[email protected]>

On space after asterisk is enough.

drivers/clk/intel/clk-cgu.c line 25

Quoted 3 lines

+ */

drivers/clk/intel/clk-cgu.c new file +2 -0

+#define to_intel_clk_divider(_hw) \
+			container_of(_hw, struct intel_clk_divider, hw)

One TAB is enough.

drivers/clk/intel/clk-cgu.c line 55

Quoted 1 line

drivers/clk/intel/clk-cgu.c new file +1 -0

+	pr_debug("Add clk: %s, id: %u\n", clk_hw_get_name(hw), id);

Is this useful?

drivers/clk/intel/clk-lgm.c line 44

Quoted 6 lines

drivers/clk/intel/clk-lgm.c new file +6 -0

+/*
+ * Below table defines the pair's of regval & effective dividers.
+ * It's more efficient to provide an explicit table due to non-linear
+ * relation between values.
+ */
+static const struct clk_div_table pll_div[] = {

Does val == 0 follows the table, i.e. makes div == 1?

Quoted 12 lines

drivers/clk/intel/clk-lgm.c new file +12 -0

+	{ .val = 1, .div = 2 },
+	{ .val = 2, .div = 3 },
+	{ .val = 3, .div = 4 },
+	{ .val = 4, .div = 5 },
+	{ .val = 5, .div = 6 },
+	{ .val = 6, .div = 8 },
+	{ .val = 7, .div = 10 },
+	{ .val = 8, .div = 12 },
+	{ .val = 9, .div = 16 },
+	{ .val = 10, .div = 20 },
+	{ .val = 11, .div = 24 },
+	{ .val = 12, .div = 32 },

drivers/clk/intel/clk-lgm.c line 64

Quoted 7 lines

drivers/clk/intel/clk-lgm.c new file +7 -0

+	{ .val = 13, .div = 40 },
+	{ .val = 14, .div = 48 },
+	{ .val = 15, .div = 64 },
+	{}
+};
+enum lgm_plls {
+	PLL0CZ, PLL0B, PLL1, PLL2, PLLPP, LJPLL3, LJPLL4

At the end you may put comma just for slightly better maintenance.

Quoted 5 lines

drivers/clk/intel/clk-lgm.c new file +5 -0

+};
+static int __init intel_lgm_cgu_probe(struct platform_device *pdev)
+{
+	struct intel_clk_provider *ctx;
+	struct regmap *map;

drivers/clk/intel/clk-lgm.c line 307

Quoted 6 lines

drivers/clk/intel/clk-lgm.c new file +6 -0

+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	int ret;
+
+	if (!np)
+		return -ENODEV;

Wouldn't the below fail?
That said, do you need this check at all?

drivers/clk/intel/clk-lgm.c line 311

Quoted 4 lines

drivers/clk/intel/clk-lgm.c new file +4 -0

+
+	map = syscon_node_to_regmap(np);
+	if (IS_ERR(map))
+		return -ENODEV;

Why shadow error code?

drivers/clk/intel/clk-lgm.c line 315

Quoted 4 lines

drivers/clk/intel/clk-lgm.c new file +4 -0

+
+	ctx = intel_clk_init(dev, map, CLK_NR_CLKS);
+	if (IS_ERR(ctx))
+		return -ENOMEM;

Ditto.

Quoted 1 line

+}

Quoted 4 lines · Andy Shevchenko

Quoted 2 lines · Rahul Tanwar

  drivers/clk/intel/Kconfig       |  13 +
  drivers/clk/intel/Makefile      |   4 +

Quoted 1 line

Any plans what to do with existing x86 folder there?

I checked the x86 folder. This driver's clock controller IP is totally different than other clock drivers inside x86. So having a common driver source is not a option. It is of course possible to move this driver inside x86 folder. Please let me know if you think moving this driver inside x86 folder makes more sense.

Quoted 10 lines

Quoted 10 lines

drivers/clk/intel/clk-cgu-pll.c new file +10 -0

+/*
+ * Calculate formula:
+ * rate = (prate * mult + (prate * frac) / frac_div) / div
+ */
+static unsigned long
+intel_pll_calc_rate(unsigned long prate, unsigned int mult,
+		    unsigned int div, unsigned int frac, unsigned int frac_div)
+{
+	u64 crate, frate, rate64;
+

Quoted 8 lines

drivers/clk/intel/clk-cgu-pll.c line 35

Quoted 4 lines

drivers/clk/intel/clk-cgu-pll.c new file +4 -0

+	rate64 = prate;
+	crate = rate64 * mult;
+
+	if (frac) {

This seems unnecessary.
I think you would like to check for frac_div instead?
Though I would rather to use frac = 0, frac_div = 1 and drop this conditional completely.

frac_div value is fixed to BIT(24) i.e. always a non zero value. mult & div are directly read from registers and by design the register values for mult & div is also always a non zero value. However, frac can logically be zero. So, I still find if (frac) condition most suitable here.

Quoted 7 lines

Quoted 7 lines

drivers/clk/intel/clk-cgu-pll.c new file +7 -0

+		frate = rate64 * frac;
+		do_div(frate, frac_div);
+		crate += frate;
+	}
+	do_div(crate, div);
+
+	return (unsigned long)crate;

Quoted 2 lines

drivers/clk/intel/clk-cgu-pll.c line 135

Quoted 1 line

drivers/clk/intel/clk-cgu-pll.c new file +1 -0

+	hw = &pll->hw;

Seems redundant temporary variable.

Agree, will update in v2.

Quoted 2 lines

drivers/clk/intel/clk-cgu.c line 55

Quoted 1 line

drivers/clk/intel/clk-cgu.c new file +1 -0

+	pr_debug("Add clk: %s, id: %u\n", clk_hw_get_name(hw), id);

Is this useful?

Yes, IMO, this proves very useful for system wide clock issues debugging during bootup.

Quoted 1 line

Does val == 0 follows the table, i.e. makes div == 1?

0 val means output clock is ref clock i.e. div ==1. Agree that adding .val = 0, .div =1 entry will make it more clear & complete.

Quoted 3 lines

drivers/clk/intel/clk-lgm.c line 46

Quoted 3 lines

drivers/clk/intel/clk-lgm.c new file +3 -0

+	{ .val = 0, .div = 1 },
+	{ .val = 1, .div = 2 },
+	{ .val = 2, .div = 3 },

Quoted 3 lines

drivers/clk/intel/clk-lgm.c line 49

Quoted 3 lines

drivers/clk/intel/clk-lgm.c new file +3 -0

+	{ .val = 3, .div = 4 },
+	{ .val = 4, .div = 5 },
+	{ .val = 5, .div = 6 },

Quoted 3 lines

drivers/clk/intel/clk-lgm.c line 52

Quoted 3 lines

drivers/clk/intel/clk-lgm.c new file +3 -0

+	{ .val = 6, .div = 8 },
+	{ .val = 7, .div = 10 },
+	{ .val = 8, .div = 12 },

Quoted 3 lines

drivers/clk/intel/clk-lgm.c line 55

Quoted 3 lines

drivers/clk/intel/clk-lgm.c new file +3 -0

+	{ .val = 9, .div = 16 },
+	{ .val = 10, .div = 20 },
+	{ .val = 11, .div = 24 },

Quoted 3 lines

drivers/clk/intel/clk-lgm.c line 58

Quoted 3 lines

drivers/clk/intel/clk-lgm.c new file +3 -0

+	{ .val = 12, .div = 32 },
+	{ .val = 13, .div = 40 },
+	{ .val = 14, .div = 48 },

Quoted 1 line

drivers/clk/intel/clk-lgm.c line 59

Quoted 1 line

drivers/clk/intel/clk-lgm.c new file +1 -0

+	{ .val = 15, .div = 64 },

Quoted 11 lines · Rahul

Quoted 3 lines · Andy Shevchenko

On Wed, Aug 28, 2019 at 03:00:17PM +0800, Rahul Tanwar wrote:

Does val == 0 follows the table, i.e. makes div == 1?

0 val means output clock is ref clock i.e. div ==1. Agree that adding .val = 0, .div =1 entry will make it more clear & complete.

Quoted 3 lines

drivers/clk/intel/clk-lgm.c line 46

Quoted 3 lines

drivers/clk/intel/clk-lgm.c new file +3 -0

+	{ .val = 0, .div = 1 },
+	{ .val = 1, .div = 2 },
+	{ .val = 2, .div = 3 },

1

Quoted 3 lines

Quoted 3 lines

drivers/clk/intel/clk-lgm.c line 49

Quoted 3 lines

drivers/clk/intel/clk-lgm.c new file +3 -0

+	{ .val = 3, .div = 4 },
+	{ .val = 4, .div = 5 },
+	{ .val = 5, .div = 6 },

1

Quoted 3 lines

Quoted 3 lines

drivers/clk/intel/clk-lgm.c line 52

Quoted 3 lines

drivers/clk/intel/clk-lgm.c new file +3 -0

+	{ .val = 6, .div = 8 },
+	{ .val = 7, .div = 10 },
+	{ .val = 8, .div = 12 },

2

Quoted 3 lines

Quoted 3 lines

drivers/clk/intel/clk-lgm.c line 55

Quoted 3 lines

drivers/clk/intel/clk-lgm.c new file +3 -0

+	{ .val = 9, .div = 16 },
+	{ .val = 10, .div = 20 },
+	{ .val = 11, .div = 24 },

4

Quoted 3 lines

Quoted 3 lines

drivers/clk/intel/clk-lgm.c line 58

Quoted 3 lines

drivers/clk/intel/clk-lgm.c new file +3 -0

+	{ .val = 12, .div = 32 },
+	{ .val = 13, .div = 40 },
+	{ .val = 14, .div = 48 },

8

Quoted 1 line

Quoted 1 line

drivers/clk/intel/clk-lgm.c line 59

Quoted 1 line

drivers/clk/intel/clk-lgm.c new file +1 -0

+	{ .val = 15, .div = 64 },

16

So, now we see the pattern:

	div = val < 3 ? (val + 1) : (1 << ((val - 3) / 3));

So, can we eliminate table?

Quoted 32 lines · Andy Shevchenko

Quoted 9 lines · Rahul

Quoted 2 lines · Andy Shevchenko

On Wed, Aug 28, 2019 at 03:00:17PM +0800, Rahul Tanwar wrote:
Does val == 0 follows the table, i.e. makes div == 1?

0 val means output clock is ref clock i.e. div ==1. Agree that adding .val = 0, .div =1 entry will make it more clear & complete.

Quoted 3 lines

drivers/clk/intel/clk-lgm.c line 46

Quoted 3 lines

drivers/clk/intel/clk-lgm.c new file +3 -0

+	{ .val = 0, .div = 1 },
+	{ .val = 1, .div = 2 },
+	{ .val = 2, .div = 3 },

1

Quoted 3 lines

Quoted 3 lines

drivers/clk/intel/clk-lgm.c line 49

Quoted 3 lines

drivers/clk/intel/clk-lgm.c new file +3 -0

+	{ .val = 3, .div = 4 },
+	{ .val = 4, .div = 5 },
+	{ .val = 5, .div = 6 },

1

Quoted 3 lines

Quoted 3 lines

drivers/clk/intel/clk-lgm.c line 52

Quoted 3 lines

drivers/clk/intel/clk-lgm.c new file +3 -0

+	{ .val = 6, .div = 8 },
+	{ .val = 7, .div = 10 },
+	{ .val = 8, .div = 12 },

2

Quoted 3 lines

Quoted 3 lines

drivers/clk/intel/clk-lgm.c line 55

Quoted 3 lines

drivers/clk/intel/clk-lgm.c new file +3 -0

+	{ .val = 9, .div = 16 },
+	{ .val = 10, .div = 20 },
+	{ .val = 11, .div = 24 },

4

Quoted 3 lines

Quoted 3 lines

drivers/clk/intel/clk-lgm.c line 58

Quoted 3 lines

drivers/clk/intel/clk-lgm.c new file +3 -0

+	{ .val = 12, .div = 32 },
+	{ .val = 13, .div = 40 },
+	{ .val = 14, .div = 48 },

8

Quoted 7 lines

Quoted 1 line

Quoted 1 line

drivers/clk/intel/clk-lgm.c line 59

Quoted 1 line

drivers/clk/intel/clk-lgm.c new file +1 -0

+	{ .val = 15, .div = 64 },

16

So, now we see the pattern:

	div = val < 3 ? (val + 1) : (1 << ((val - 3) / 3));

It's not complete, but I think you got the idea.

Quoted 1 line

So, can we eliminate table?

Quoted 36 lines · Andy Shevchenko

Quoted 35 lines · Andy Shevchenko

Quoted 6 lines · Rahul

Quoted 5 lines · Andy Shevchenko

Quoted 3 lines · Rahul Tanwar

+ { .val = 0, .div = 1 },
+ { .val = 1, .div = 2 },
+ { .val = 2, .div = 3 },

1

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

+ { .val = 3, .div = 4 },
+ { .val = 4, .div = 5 },
+ { .val = 5, .div = 6 },

1

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

+ { .val = 6, .div = 8 },
+ { .val = 7, .div = 10 },
+ { .val = 8, .div = 12 },

2

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

+ { .val = 9, .div = 16 },
+ { .val = 10, .div = 20 },
+ { .val = 11, .div = 24 },

4

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

+ { .val = 12, .div = 32 },
+ { .val = 13, .div = 40 },
+ { .val = 14, .div = 48 },

8

Quoted 1 line

Quoted 1 line

Quoted 1 line

+ { .val = 15, .div = 64 },

16

So, now we see the pattern:

Quoted 4 lines

Quoted 1 line

     div = val < 3 ? (val + 1) : (1 << ((val - 3) / 3));

It's not complete, but I think you got the idea.

Quoted 1 line

So, can we eliminate table?

In the desperation to eliminate table, below is what i can come up with:

        struct clk_div_table div_table[16];

        int i, j;

        for (i = 0; i < 16; i++)
                div_table[i].val = i;

        for (i = 0, j=0; i < 16; i+=3, j++) {
                div_table[i].div = (i == 0) ? (1 << j) : (1 << (j + 1));
                if (i == 15)
                        break;

                div_table[i + 1].div = (i == 0) ? ((1 << j) + 1) :
                                        (1 << (j + 1)) + (1 << (j - 1));
                div_table[i + 2].div = (3 << j);
        }

To me, table still looks a better approach. Also, table is more extendable & consistent w.r.t. clk framework & other referenced clk drivers.

Whats your opinion ?

Quoted 10 lines · Rahul

Quoted 5 lines · Andy Shevchenko

Quoted 2 lines

     div = val < 3 ? (val + 1) : (1 << ((val - 3) / 3));

It's not complete, but I think you got the idea.

Quoted 1 line

So, can we eliminate table?

In the desperation to eliminate table, below is what i can come up with:

        struct clk_div_table div_table[16];

But this is not an elimination, it's just a replacement from static to dynamically calculated one.

Quoted 15 lines

        int i, j;

        for (i = 0; i < 16; i++)
                div_table[i].val = i;

        for (i = 0, j=0; i < 16; i+=3, j++) {
                div_table[i].div = (i == 0) ? (1 << j) : (1 << (j + 1));
                if (i == 15)
                        break;

                div_table[i + 1].div = (i == 0) ? ((1 << j) + 1) :
                                        (1 << (j + 1)) + (1 << (j - 1));
                div_table[i + 2].div = (3 << j);
        }

Quoted 4 lines

To me, table still looks a better approach. Also, table is more extendable & consistent w.r.t. clk framework & other referenced clk drivers.

Whats your opinion ?

Whatever CCF maintainers is fine with.

Hello,

I only noticed this patchset today and I don't have much time left. Here's my initial impressions without going through the code in detail. I'll continue my review in the next days (as time permits).

As with all other Intel LGM patches: I don't have access to the datasheets, so it's possible that I don't understand <insert topic here> feel free to correct me in this case (I appreciate an explanation where I was wrong, so I can learn from it)

[...]

drivers/clk/intel/Kconfig new file +4 -0

@@ -0,0 +1,13 @@
+# SPDX-License-Identifier: GPL-2.0
+config INTEL_LGM_CGU_CLK
+	depends on COMMON_CLK
+	select MFD_SYSCON

can you please explain the reason why you need to use syscon? also please see [0] for a comment from Rob on another LGM dt-binding regarding syscon

drivers/clk/intel/Kconfig new file +1 -0

+	select OF_EARLY_FLATTREE

there's not a single other "select OF_EARLY_FLATTREE" in driver/clk I'm not saying this is wrong but it makes me curious why you need this

[...]

drivers/clk/intel/clk-cgu.h new file +54 -0

new file mode 100644
index 000000000000..e44396b4aad7
@@ -0,0 +1,278 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ *  Copyright(c) 2018 Intel Corporation.
+ *  Zhu YiXin <[email protected]>
+ */
+
+#ifndef __INTEL_CLK_H
+#define __INTEL_CLK_H
+
+struct intel_clk_mux {
+	struct clk_hw hw;
+	struct device *dev;
+	struct regmap *map;
+	unsigned int reg;
+	u8 shift;
+	u8 width;
+	unsigned long flags;
+};
+
+struct intel_clk_divider {
+	struct clk_hw hw;
+	struct device *dev;
+	struct regmap *map;
+	unsigned int reg;
+	u8 shift;
+	u8 width;
+	unsigned long flags;
+	const struct clk_div_table *table;
+};
+
+struct intel_clk_ddiv {
+	struct clk_hw hw;
+	struct device *dev;
+	struct regmap *map;
+	unsigned int reg;
+	u8 shift0;
+	u8 width0;
+	u8 shift1;
+	u8 width1;
+	u8 shift2;
+	u8 width2;
+	unsigned int mult;
+	unsigned int div;
+	unsigned long flags;
+};
+
+struct intel_clk_gate {
+	struct clk_hw hw;
+	struct device *dev;
+	struct regmap *map;
+	unsigned int reg;
+	u8 shift;
+	unsigned long flags;
+};

I know at least two existing regmap clock implementations:
- drivers/clk/qcom/clk-regmap*
- drivers/clk/meson/clk-regmap*

it would be great if we could decide to re-use one of those for the "generic" clock types (mux, divider and gate).
Stephen, do you have any preference here?
personally I like the meson one, but I'm biased because I've used it a lot in the past and I haven't used the qcom one at all.

Hi Martin,

Quoted 21 lines · Martin Blumenstingl

Hello,

I only noticed this patchset today and I don't have much time left. Here's my initial impressions without going through the code in detail. I'll continue my review in the next days (as time permits).

As with all other Intel LGM patches: I don't have access to the datasheets, so it's possible that I don't understand <insert topic here> feel free to correct me in this case (I appreciate an explanation where I was wrong, so I can learn from it)

[...]

drivers/clk/intel/Kconfig new file +4 -0

@@ -0,0 +1,13 @@
+# SPDX-License-Identifier: GPL-2.0
+config INTEL_LGM_CGU_CLK
+     depends on COMMON_CLK
+     select MFD_SYSCON

can you please explain the reason why you need to use syscon?

Quoted 2 lines

also please see [0] for a comment from Rob on another LGM dt-binding regarding syscon

Actually, there is no need to use syscon for CGU in LGM. It got carried over from older SoCs (Falcon Mountain) where CGU was a MFD device because it included PHY registers as well. And PHY drivers were using syscon node to access CGU regmap. But for LGM, this is not the case.

My understanding is that if we do not use syscon, then there is no point in using regmap because this driver uses simple 32 bit register access. Can directly read/write registers using readl() & writel().

Would you agree ?

Quoted 3 lines

+ select OF_EARLY_FLATTREE
there's not a single other "select OF_EARLY_FLATTREE" in driver/clk I'm not saying this is wrong but it makes me curious why you need this

We need OF_EARLY_FLATTREE for LGM. But adding a new x86 platform for LGM is discouraged because that would lead to too many platforms. Only differentiating factor for LGM is CPU model ID but it can differentiate only at run time. So i had no option other then enabling it with some LGM specific core system module driver and CGU seemed perfect for this purpose.

Quoted 64 lines

drivers/clk/intel/clk-cgu.h new file +54 -0

new file mode 100644
index 000000000000..e44396b4aad7
@@ -0,0 +1,278 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ *  Copyright(c) 2018 Intel Corporation.
+ *  Zhu YiXin <[email protected]>
+ */
+
+#ifndef __INTEL_CLK_H
+#define __INTEL_CLK_H
+
+struct intel_clk_mux {
+     struct clk_hw hw;
+     struct device *dev;
+     struct regmap *map;
+     unsigned int reg;
+     u8 shift;
+     u8 width;
+     unsigned long flags;
+};
+
+struct intel_clk_divider {
+     struct clk_hw hw;
+     struct device *dev;
+     struct regmap *map;
+     unsigned int reg;
+     u8 shift;
+     u8 width;
+     unsigned long flags;
+     const struct clk_div_table *table;
+};
+
+struct intel_clk_ddiv {
+     struct clk_hw hw;
+     struct device *dev;
+     struct regmap *map;
+     unsigned int reg;
+     u8 shift0;
+     u8 width0;
+     u8 shift1;
+     u8 width1;
+     u8 shift2;
+     u8 width2;
+     unsigned int mult;
+     unsigned int div;
+     unsigned long flags;
+};
+
+struct intel_clk_gate {
+     struct clk_hw hw;
+     struct device *dev;
+     struct regmap *map;
+     unsigned int reg;
+     u8 shift;
+     unsigned long flags;
+};

I know at least two existing regmap clock implementations:
- drivers/clk/qcom/clk-regmap*
- drivers/clk/meson/clk-regmap*

Quoted 5 lines

it would be great if we could decide to re-use one of those for the "generic" clock types (mux, divider and gate).
Stephen, do you have any preference here?
personally I like the meson one, but I'm biased because I've used it a lot in the past and I haven't used the qcom one at all.

I went through meson & qcom regmap clock code. Agree, it can be reused for mux, divider and gate. But as mentioned above, i am now considering to move away from using regmap.

Quoted 5 lines

My understanding is that if we do not use syscon, then there is no point in using regmap because this driver uses simple 32 bit register access. Can directly read/write registers using readl() & writel().

Would you agree ?

if there was only the LGM SoC then I would say: drop regmap

however, last year a driver for the GRX350/GRX550 SoCs was proposed: [0] this was never updated but it seems to use the same "framework" as the LGM driver
with this in mind I am for keeping regmap support because.
I think it will be easier to add support for old SoCs like GRX350/GRX550 (but also VRX200), because the PLL sub-driver (I am assuming that it is similar on all SoCs) or some other helpers can be re-used across various SoCs instead of "duplicating" code (where one variant would use regmap and the other readl/writel).

[...]

Quoted 10 lines

Quoted 3 lines

+ select OF_EARLY_FLATTREE
there's not a single other "select OF_EARLY_FLATTREE" in driver/clk I'm not saying this is wrong but it makes me curious why you need this

We need OF_EARLY_FLATTREE for LGM. But adding a new x86 platform for LGM is discouraged because that would lead to too many platforms. Only differentiating factor for LGM is CPU model ID but it can differentiate only at run time. So i had no option other then enabling it with some LGM specific core system module driver and CGU seemed perfect for this purpose.

so when my x86 kernel maintainer enables CONFIG_INTEL_LGM_CGU_CLK then OF_EARLY_FLATTREE is enabled as well.
does this hurt any existing x86 platform? if not: why can't we enable it for x86 unconditionally?

I went through meson & qcom regmap clock code. Agree, it can be reused for mux, divider and gate. But as mentioned above, i am now considering to move away from using regmap.
thank you for evaluating them. let's continue the discussion above whether regmap should be used - after that we decide (if needed) which regmap implementation to use

Martin

[0] https://patchwork.kernel.org/patch/10554401/

Hi Martin,

Quoted 7 lines · Martin Blumenstingl

Quoted 5 lines

My understanding is that if we do not use syscon, then there is no point in using regmap because this driver uses simple 32 bit register access. Can directly read/write registers using readl() & writel().

Would you agree ?

if there was only the LGM SoC then I would say: drop regmap

Quoted 9 lines

however, last year a driver for the GRX350/GRX550 SoCs was proposed: [0] this was never updated but it seems to use the same "framework" as the LGM driver
with this in mind I am for keeping regmap support because.
I think it will be easier to add support for old SoCs like GRX350/GRX550 (but also VRX200), because the PLL sub-driver (I am assuming that it is similar on all SoCs) or some other helpers can be re-used across various SoCs instead of "duplicating" code (where one variant would use regmap and the other readl/writel).

Earlier, we had discussed about it in our team. There are no plans to upstream mips based platform code, past up-streaming efforts for mips platforms were also dropped. GRX350/GRX550/VRX200 are all mips based platforms. Plan is to upstream only x86 based platforms. In-fact, i had removed GRX & other older SoCs support from this driver before sending for review. So we can consider only x86 based LGM family of SoCs for this driver & all of them will be reusing same IP.

Hi Rahul,

Quoted 21 lines · Rahul

Hi Martin,

Quoted 7 lines · Martin Blumenstingl

Quoted 5 lines

My understanding is that if we do not use syscon, then there is no point in using regmap because this driver uses simple 32 bit register access. Can directly read/write registers using readl() & writel().

Would you agree ?

if there was only the LGM SoC then I would say: drop regmap

Quoted 9 lines

however, last year a driver for the GRX350/GRX550 SoCs was proposed: [0] this was never updated but it seems to use the same "framework" as the LGM driver
with this in mind I am for keeping regmap support because.
I think it will be easier to add support for old SoCs like GRX350/GRX550 (but also VRX200), because the PLL sub-driver (I am assuming that it is similar on all SoCs) or some other helpers can be re-used across various SoCs instead of "duplicating" code (where one variant would use regmap and the other readl/writel).

Quoted 7 lines

Earlier, we had discussed about it in our team. There are no plans to upstream mips based platform code, past up-streaming efforts for mips platforms were also dropped. GRX350/GRX550/VRX200 are all mips based platforms. Plan is to upstream only x86 based platforms. In-fact, i had removed GRX & other older SoCs support from this driver before sending for review. So we can consider only x86 based LGM family of SoCs for this driver & all of them will be reusing same IP.

this is very sad news
as far as I can tell many IP cores are similar/identical on GRX350/GRX550, LGM and even VRX200

I already know that VRX200 is a legacy product and you won't be supporting it once LGM support lands upstream you could add support for GRX350/GRX550 with small to medium effort
that is a big win (in my opinion) because it means happier end-users (see XWAY and VRX200 support in OpenWrt for example: while support from Intel/Lantiq has died long ago these devices can still run a recent LTS kernel and get security updates. without OpenWrt these devices would probably end up as electronic waste)

maybe implementing a re-usable regmap clock driver (for mux, gate and divider) means less effort (compared to converting everything to standard clock ops) for you.
(we did the switch from standard clock ops to regmap for the Amlogic Meson clock drivers when we discovered that there were some non-clock registers that belong to other IP blocks in it and it was a lot of effort)
this will allow you to add support for GRX350/GRX550 in the future if demand for upstream drivers rises.

Martin

I've discussed internally the amount of efforts to create a reusable regmap clock driver which might be reused by other companies too.

It seems it requires significant efforts for implementation/tests. As we don't plan to support our old SOCs for now, I'm not sure if we need to put such a big efforts.

Stephan,

It seems you don't like both meson/qcom regmap clock implementation.

What is your opinion for our current CGU clock driver implementation?

drivers/clk/intel/clk-cgu.h new file +8 -0

+struct intel_clk_gate {
+       struct clk_hw hw;
+       struct device *dev;
+       struct regmap *map;
+       unsigned int reg;
+       u8 shift;
+       unsigned long flags;
+};

I know at least two existing regmap clock implementations:
- drivers/clk/qcom/clk-regmap*
- drivers/clk/meson/clk-regmap*

it would be great if we could decide to re-use one of those for the "generic" clock types (mux, divider and gate).
Stephen, do you have any preference here?
personally I like the meson one, but I'm biased because I've used it a lot in the past and I haven't used the qcom one at all.

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.