lkml.org 
[lkml]   [2019]   [Aug]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v1 1/2] clk: intel: Add CGU clock driver for a new SoC
On Wed, Aug 28, 2019 at 03:00:17PM +0800, Rahul Tanwar wrote:
> From: rtanwar <rahul.tanwar@intel.com>
>
> 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 <Yixin.zhu@intel.com>

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


\
 
 \ /
  Last update: 2019-08-28 17:10    [W:0.082 / U:2.000 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site