Re: [PATCH v1 1/2] clk: intel: Add CGU clock driver for a new SoC
From: Tanwar, Rahul
Date: Mon Sep 02 2019 - 03:43:21 EST
Hi Andy,
Thanks for your review comments.
On 28/8/2019 11:09 PM, Andy Shevchenko wrote:
On Wed, Aug 28, 2019 at 03:00:17PM +0800, Rahul Tanwar wrote:
From: rtanwar <rahul.tanwar@xxxxxxxxx>
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?
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.
+++ b/drivers/clk/intel/Kconfig
@@ -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?
Yes agree, will update in v2.
+/*
+ * 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;
+
+ 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.
+ 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.
Yes, will update in v2.
+ 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;
+ 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.
Agree, will update in v2.
+ 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.
Will fix in v2.
+{
+ 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?
Agree that its fatal if PLL registration fails, just that its highly
unlikely.
I will modify it to return error & make probe fail in failure cases.
+ continue;
+ }
+
+ intel_clk_add_lookup(ctx, hw, list->id);
+ }
No error to return? Are all PLLs optional?
Will change it to return error code.
+}
+#endif /* __INTEL_CLK_PLL_H */
One TAB is enough.
Will fix in v2.
+/*
+ * Copyright (C) 2018 Intel Corporation.
+ * Zhu YiXin <Yixin.zhu@xxxxxxxxx>
On space after asterisk is enough.
Will fix in v2.
+ */
+#define to_intel_clk_divider(_hw) \
+ container_of(_hw, struct intel_clk_divider, hw)
One TAB is enough.
Will fix in v2.
+ 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.
+/*
+ * 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?
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.
+ { .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 },
+ { .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.
Sure, will add.
+};
+static int __init intel_lgm_cgu_probe(struct platform_device *pdev)
+{
+ struct intel_clk_provider *ctx;
+ struct regmap *map;
+ 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?
Agree, that this check is not needed. Will update in v2.
+
+ map = syscon_node_to_regmap(np);
+ if (IS_ERR(map))
+ return -ENODEV;
Why shadow error code?
Yes, will change in v2.
+
+ ctx = intel_clk_init(dev, map, CLK_NR_CLKS);
+ if (IS_ERR(ctx))
+ return -ENOMEM;
Ditto.
Will change in v2.
Regards,
Rahul
+}