On Wed, Aug 28, 2019 at 03:00:17PM +0800, Rahul Tanwar wrote:
> 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?
> +++ 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?
> +/*
> + * 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.
> + 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.
> + 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.
> + 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.
> +{
> + 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?
> + continue;
> + }
> +
> + intel_clk_add_lookup(ctx, hw, list->id);
> + }
No error to return? Are all PLLs optional?
> +}
> +#endif /* __INTEL_CLK_PLL_H */
One TAB is enough.
> +/*
> + * Copyright (C) 2018 Intel Corporation.
> + * Zhu YiXin <[email protected]>
On space after asterisk is enough.
> + */
> +#define to_intel_clk_divider(_hw) \
> + container_of(_hw, struct intel_clk_divider, hw)
One TAB is enough.
> + val >>= shift;
> + val &= BIT(width) - 1;
> +
> + return val;
Can be one line, though up to you.
> + pr_debug("Add clk: %s, id: %u\n", clk_hw_get_name(hw), id);
Is this useful?
> +static struct clk_hw
> +*intel_clk_register_fixed(struct intel_clk_provider *ctx,
* is part of the type.
> + const struct intel_clk_branch *list)
> +static struct clk_hw
> +*intel_clk_register_fixed_factor(struct intel_clk_provider *ctx,
Ditto.
> + const struct intel_clk_branch *list)
> +static struct clk_hw
> +*intel_clk_register_gate(struct intel_clk_provider *ctx,
Ditto.
> + const struct intel_clk_branch *list)
> +/*
> + * 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?
> + { .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.
> +};
> +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?
> +
> + map = syscon_node_to_regmap(np);
> + if (IS_ERR(map))
> + return -ENODEV;
Why shadow error code?
> +
> + ctx = intel_clk_init(dev, map, CLK_NR_CLKS);
> + if (IS_ERR(ctx))
> + return -ENOMEM;
Ditto.
> +}
--
With Best Regards,
Andy Shevchenko
