From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 873A9C433DF for ; Fri, 24 Jul 2020 08:43:02 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 470E320674 for ; Fri, 24 Jul 2020 08:43:02 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="20tn0lPJ"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="z3Cw2F0S" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 470E320674 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:Message-ID:Date:To:From:Subject:References: In-Reply-To:MIME-Version:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=PlpPbDyhzE8PsKFPXjDb85Lrm2dgIrtp/jc0Ui0c9+o=; b=20tn0lPJ0KZmdtcwdAp6Yvzyc 0biEAx6bkVj0pwSbjvu6jqNRY9XOT6Iof0nyoZb3GzFf+b+oTpIrk/BtGyWzd8+tvGMnA3mXS8Lhp Od5k2amR+ouhzR6O58Gw8iYvuYLZCvHULtua5pA3160xQC6JQV3HqsbXrggNBEHATrGIw1EiWIgY3 2yqu5yqlrnWg6c86oIcq+9D/lFoXkwQ6Tq9rrnGVqlOr09ROEuoltiRD7NNpW2h3DqWzeTJp3ir70 2IQZxNRcrLLMAMnpCoR3/Dd5h338QgQwoZuCecUE+/6yq/W3Mu9LdpyIynVrWnNpLGHfe9fnUUn66 5xCB77fqw==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jytGo-00018g-HG; Fri, 24 Jul 2020 08:41:34 +0000 Received: from mail.kernel.org ([198.145.29.99]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jytGl-00017N-Mo for linux-arm-kernel@lists.infradead.org; Fri, 24 Jul 2020 08:41:32 +0000 Received: from kernel.org (unknown [104.132.0.74]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 788EB20674; Fri, 24 Jul 2020 08:41:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1595580090; bh=n9jHH7Aw7/UhgmDWUC7DirxYOE9kLQl65+9MTWAXmiw=; h=In-Reply-To:References:Subject:From:Cc:List-Id:To:Date:From; b=z3Cw2F0S7gMDG7zMSi7cPVtj6XKZUEhyj8nzXvJcaVRXzo4I6dHymkJ1HjSnsFU2u W9jltmh4FNzuUIgwhcM70j56MWv3ypYhPw3i995FMf/nDX/AdjZp3WOJyj7uzKfNPp dJtM1U5OsRyW45bhgvLnUV+O0XWKMfEpjYES9v3M= MIME-Version: 1.0 In-Reply-To: <20200615133242.24911-9-lars.povlsen@microchip.com> References: <20200615133242.24911-1-lars.povlsen@microchip.com> <20200615133242.24911-9-lars.povlsen@microchip.com> Subject: Re: [PATCH v3 08/10] clk: sparx5: Add Sparx5 SoC DPLL clock driver From: Stephen Boyd List-Id: To: Arnd Bergmann , Lars Povlsen , Linus Walleij , SoC Team Date: Fri, 24 Jul 2020 01:41:29 -0700 Message-ID: <159558008977.3847286.10561464126267966931@swboyd.mtv.corp.google.com> User-Agent: alot/0.9.1 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200724_044131_887495_F5E3FC21 X-CRM114-Status: GOOD ( 28.96 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: devicetree@vger.kernel.org, Alexandre Belloni , Steen Hegelund , linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org, Microchip Linux Driver Support , linux-gpio@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Olof Johansson , Michael Turquette , Lars Povlsen Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Quoting Lars Povlsen (2020-06-15 06:32:40) > diff --git a/drivers/clk/clk-sparx5.c b/drivers/clk/clk-sparx5.c > new file mode 100644 > index 0000000000000..c2e7aa0214ebd > --- /dev/null > +++ b/drivers/clk/clk-sparx5.c > @@ -0,0 +1,312 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Microchip Sparx5 SoC Clock driver. > + * > + * Copyright (c) 2019 Microchip Inc. > + * > + * Author: Lars Povlsen > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include Is this include used? > +#include > +#include > +#include > + > +#define PLL_DIV GENMASK(7, 0) > +#define PLL_PRE_DIV GENMASK(10, 8) > +#define PLL_ROT_DIR BIT(11) > +#define PLL_ROT_SEL GENMASK(13, 12) > +#define PLL_ROT_ENA BIT(14) > +#define PLL_CLK_ENA BIT(15) > + > +#define MAX_SEL 4 > +#define MAX_PRE BIT(3) > + > +static const u8 sel_rates[MAX_SEL] = { 0, 2*8, 2*4, 2*2 }; > + > +static const char *clk_names[N_CLOCKS] = { > + "core", "ddr", "cpu2", "arm2", > + "aux1", "aux2", "aux3", "aux4", > + "synce", > +}; > + > +struct s5_hw_clk { > + struct clk_hw hw; > + void __iomem *reg; > + int index; This looks unused. Drop it? > +}; > + > +struct s5_clk_data { > + void __iomem *base; > + struct s5_hw_clk s5_hw[N_CLOCKS]; > +}; > + > +struct s5_pll_conf { > + int freq; Why not unsigned long like the type that s5_calc_freq() returns? > + u8 div; > + bool rot_ena; > + u8 rot_sel; > + u8 rot_dir; > + u8 pre_div; > +}; > + > +#define to_s5_pll(hw) container_of(hw, struct s5_hw_clk, hw) > + > +static unsigned long s5_calc_freq(unsigned long parent_rate, > + const struct s5_pll_conf *conf) > +{ > + unsigned long rate = parent_rate / conf->div; > + > + if (conf->rot_ena) { > + int sign = conf->rot_dir ? -1 : 1; > + int divt = sel_rates[conf->rot_sel] * (1 + conf->pre_div); > + int divb = divt + sign; > + > + rate = mult_frac(rate, divt, divb); > + rate = roundup(rate, 1000); > + } > + > + return rate; > +} > + > +static void s5_search_fractional(unsigned long rate, > + unsigned long parent_rate, > + int div, > + struct s5_pll_conf *conf) > +{ > + struct s5_pll_conf best; > + ulong cur_offset, best_offset = rate; > + int d, i, j; > + > + memset(conf, 0, sizeof(*conf)); > + conf->div = div; > + conf->rot_ena = 1; /* Fractional rate */ > + > + for (d = 0; best_offset > 0 && d <= 1 ; d++) { > + conf->rot_dir = !!d; > + for (i = 0; best_offset > 0 && i < MAX_PRE; i++) { > + conf->pre_div = i; > + for (j = 1; best_offset > 0 && j < MAX_SEL; j++) { > + conf->rot_sel = j; > + conf->freq = s5_calc_freq(parent_rate, conf); > + cur_offset = abs(rate - conf->freq); > + if (cur_offset < best_offset) { > + best_offset = cur_offset; > + best = *conf; > + } > + } > + } > + } > + > + /* Best match */ > + *conf = best; > +} > + > +static unsigned long s5_calc_params(unsigned long rate, > + unsigned long parent_rate, > + struct s5_pll_conf *conf) > +{ > + if (parent_rate % rate) { > + struct s5_pll_conf alt1, alt2; > + int div; > + > + div = DIV_ROUND_CLOSEST_ULL(parent_rate, rate); > + s5_search_fractional(rate, parent_rate, div, &alt1); > + > + /* Straight match? */ > + if (alt1.freq == rate) { > + *conf = alt1; > + } else { > + /* Try without rounding divider */ > + div = parent_rate / rate; > + if (div != alt1.div) { > + s5_search_fractional(rate, parent_rate, div, > + &alt2); > + /* Select the better match */ > + if (abs(rate - alt1.freq) < > + abs(rate - alt2.freq)) > + *conf = alt1; > + else > + *conf = alt2; > + } > + } > + } else { > + /* Straight fit */ > + memset(conf, 0, sizeof(*conf)); > + conf->div = parent_rate / rate; > + } > + > + return conf->freq; > +} > + > +static int s5_pll_enable(struct clk_hw *hw) > +{ > + struct s5_hw_clk *pll = to_s5_pll(hw); > + u32 val = readl(pll->reg); > + > + val |= PLL_CLK_ENA; > + writel(val, pll->reg); > + > + return 0; > +} > + > +static void s5_pll_disable(struct clk_hw *hw) > +{ > + struct s5_hw_clk *pll = to_s5_pll(hw); > + u32 val = readl(pll->reg); > + > + val &= ~PLL_CLK_ENA; > + writel(val, pll->reg); > +} > + > +static int s5_pll_set_rate(struct clk_hw *hw, > + unsigned long rate, > + unsigned long parent_rate) > +{ > + struct s5_hw_clk *pll = to_s5_pll(hw); > + struct s5_pll_conf conf; > + unsigned long eff_rate; > + u32 val; > + > + eff_rate = s5_calc_params(rate, parent_rate, &conf); > + if (eff_rate != rate) > + return -EOPNOTSUPP; > + > + val = readl(pll->reg) & PLL_CLK_ENA; > + val |= FIELD_PREP(PLL_DIV, conf.div); > + if (conf.rot_ena) { > + val |= PLL_ROT_ENA; > + val |= FIELD_PREP(PLL_ROT_SEL, conf.rot_sel); > + val |= FIELD_PREP(PLL_PRE_DIV, conf.pre_div); > + if (conf.rot_dir) > + val |= PLL_ROT_DIR; > + } > + writel(val, pll->reg); > + > + return 0; > +} > + > +static unsigned long s5_pll_recalc_rate(struct clk_hw *hw, > + unsigned long parent_rate) > +{ > + struct s5_hw_clk *pll = to_s5_pll(hw); > + struct s5_pll_conf conf; > + u32 val; > + > + val = readl(pll->reg); > + > + if (val & PLL_CLK_ENA) { > + conf.div = FIELD_GET(PLL_DIV, val); > + conf.pre_div = FIELD_GET(PLL_PRE_DIV, val); > + conf.rot_ena = FIELD_GET(PLL_ROT_ENA, val); > + conf.rot_dir = FIELD_GET(PLL_ROT_DIR, val); > + conf.rot_sel = FIELD_GET(PLL_ROT_SEL, val); > + > + conf.freq = s5_calc_freq(parent_rate, &conf); > + } else > + conf.freq = 0; Nitpick: Please add braces on single line else statements when the if is multiline. > + > + return conf.freq; > +} > + > +static long s5_pll_round_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long *parent_rate) > +{ > + struct s5_pll_conf conf; > + > + return s5_calc_params(rate, *parent_rate, &conf); > +} > + > +static const struct clk_ops s5_pll_ops = { > + .enable = s5_pll_enable, > + .disable = s5_pll_disable, > + .set_rate = s5_pll_set_rate, > + .round_rate = s5_pll_round_rate, > + .recalc_rate = s5_pll_recalc_rate, > +}; > + > +static struct clk_hw *s5_clk_hw_get(struct of_phandle_args *clkspec, void *data) > +{ > + struct s5_clk_data *s5_clk = data; > + unsigned int idx = clkspec->args[0]; > + > + if (idx >= N_CLOCKS) { > + pr_err("%s: invalid index %u\n", __func__, idx); > + return ERR_PTR(-EINVAL); > + } > + > + return &s5_clk->s5_hw[idx].hw; > +} > + > +static int s5_clk_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + int i, ret; > + struct s5_clk_data *s5_clk; > + const char *parent_name; > + struct clk_init_data init = { > + .ops = &s5_pll_ops, > + .parent_names = &parent_name, It looks like with the binding you can drop parent_name and just use .parent_data = { .index = 0 } and event drop the .index = 0 bit because that's the default valjue. > + .num_parents = 1, > + }; > + > + s5_clk = devm_kzalloc(dev, sizeof(*s5_clk), GFP_KERNEL); > + if (!s5_clk) > + return -ENOMEM; > + > + s5_clk->base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(s5_clk->base)) > + return PTR_ERR(s5_clk->base); > + > + parent_name = of_clk_get_parent_name(np, 0); > + if (!parent_name) { > + dev_err(dev, "%pOFn: missing parent clock\n", np); > + return -EINVAL; > + } It's nice because then this call goes away. > + > + for (i = 0; i < N_CLOCKS; i++) { > + struct s5_hw_clk *s5_hw = &s5_clk->s5_hw[i]; > + > + init.name = clk_names[i]; > + s5_hw->index = i; > + s5_hw->reg = s5_clk->base + (i * sizeof(u32)); I'd prefer i * 4 because the hardware engineers probably don't care about the size of a u32 in bytes. > + s5_hw->hw.init = &init; > + ret = devm_clk_hw_register(dev, &s5_hw->hw); > + if (ret) { > + dev_err(dev, "failed to register %s clock\n", > + init.name); init.name will be destroyed. Just drop this error message? Maybe we should add it into the core framework because quite a few driver authors want it. > + return ret; > + } > + } > + > + return of_clk_add_hw_provider(np, s5_clk_hw_get, s5_clk); Use devm_of_clk_add_hw_provider()? > +} > + > +static int s5_clk_remove(struct platform_device *pdev) > +{ > + of_clk_del_provider(pdev->dev.of_node); > + > + return 0; > +} > + And then remove this whole remove function? > +static const struct of_device_id s5_clk_dt_ids[] = { > + { .compatible = "microchip,sparx5-dpll", }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, s5_clk_dt_ids); > + > +static struct platform_driver s5_clk_driver = { > + .probe = s5_clk_probe, > + .remove = s5_clk_remove, > + .driver = { > + .name = "sparx5-clk", > + .of_match_table = s5_clk_dt_ids, > + }, > +}; _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel