lkml.org 
[lkml]   [2019]   [Sep]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH v1 1/2] clk: intel: Add CGU clock driver for a new SoC
From
Date

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@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?


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@intel.com>
> 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

>> +}

\
 
 \ /
  Last update: 2019-09-02 09:44    [W:0.196 / U:4.424 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site