From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755021AbcHXI6L (ORCPT ); Wed, 24 Aug 2016 04:58:11 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:53503 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754808AbcHXI5m (ORCPT ); Wed, 24 Aug 2016 04:57:42 -0400 Date: Wed, 24 Aug 2016 01:47:38 -0700 From: Stephen Boyd To: =?utf-8?B?UmFmYcWCIE1pxYJlY2tp?= Cc: Michael Turquette , linux-clk@vger.kernel.org, bcm-kernel-feedback-list@broadcom.com, =?utf-8?B?UmFmYcWCIE1pxYJlY2tp?= , Rob Herring , Mark Rutland , Eric Anholt , Jon Mason , Florian Fainelli , Stephen Warren , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , open list Subject: Re: [PATCH V5] clk: bcm: Add driver for BCM53573 ILP clock Message-ID: <20160824084738.GB6502@codeaurora.org> References: <20160823061745.8162-1-zajec5@gmail.com> <20160823062613.13865-1-zajec5@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20160823062613.13865-1-zajec5@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 08/23, Rafał Miłecki wrote: > diff --git a/drivers/clk/bcm/clk-bcm53573-ilp.c b/drivers/clk/bcm/clk-bcm53573-ilp.c > new file mode 100644 > index 0000000..b7ac0eb > --- /dev/null > +++ b/drivers/clk/bcm/clk-bcm53573-ilp.c > @@ -0,0 +1,146 @@ > +/* > + * Copyright (C) 2016 Rafał Miłecki > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#include Is this include used? > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define PMU_XTAL_FREQ_RATIO 0x66c > +#define XTAL_ALP_PER_4ILP 0x00001fff > +#define XTAL_CTL_EN 0x80000000 > +#define PMU_SLOW_CLK_PERIOD 0x6dc > + > +struct bcm53573_ilp { > + struct clk *clk; > + struct clk_hw hw; > + void __iomem *pmu; > +}; > + > +static int bcm53573_ilp_enable(struct clk_hw *hw) > +{ > + struct bcm53573_ilp *ilp = container_of(hw, struct bcm53573_ilp, hw); > + > + writel(0x10199, ilp->pmu + PMU_SLOW_CLK_PERIOD); > + writel(0x10000, ilp->pmu + 0x674); Is there a name for 0x674? > + > + return 0; > +} > + > +static unsigned long bcm53573_ilp_recalc_rate(struct clk_hw *hw, > + unsigned long parent_rate) > +{ > + struct bcm53573_ilp *ilp = container_of(hw, struct bcm53573_ilp, hw); > + void __iomem *pmu = ilp->pmu; > + u32 last_val, cur_val; > + u32 sum = 0, num = 0, loop_num = 0; Should these just be plain ints? Do we care about sizes for these variables? > + u32 avg; This one too. > + > + /* Enable measurement */ > + writel(XTAL_CTL_EN, pmu + PMU_XTAL_FREQ_RATIO); > + > + /* Read initial value */ > + last_val = readl(pmu + PMU_XTAL_FREQ_RATIO) & XTAL_ALP_PER_4ILP; > + > + /* > + * At minimum we should loop for a bit to let hardware do the > + * measurement. This isn't very accurate however, so for a better > + * precision lets try getting 20 different values for and use average. > + */ > + while (num < 20) { > + cur_val = readl(pmu + PMU_XTAL_FREQ_RATIO) & XTAL_ALP_PER_4ILP; > + > + if (cur_val != last_val) { > + /* Got different value, use it */ > + sum += cur_val; > + num++; > + loop_num = 0; > + last_val = cur_val; > + } else if (++loop_num > 5000) { > + /* Same value over and over, give up */ > + sum += cur_val; > + num++; > + break; > + } Should there be a udelay() here? Or we're expected to tight loop read the hardware? If so we should throw in a cpu_relax() here to indicate tight loop. > + } > + > + /* Disable measurement to save power */ > + writel(0x0, pmu + PMU_XTAL_FREQ_RATIO); > + > + avg = sum / num; > + > + return parent_rate * 4 / avg; > +} > + > +static const struct clk_ops bcm53573_ilp_clk_ops = { > + .enable = bcm53573_ilp_enable, No disable? Or .is_enabled? > + .recalc_rate = bcm53573_ilp_recalc_rate, > +}; > + > +static void bcm53573_ilp_init(struct device_node *np) > +{ > + struct bcm53573_ilp *ilp; > + struct resource res; > + struct clk_init_data init = { 0 }; > + const char *parent_name; > + int index; > + int err; > + > + ilp = kzalloc(sizeof(*ilp), GFP_KERNEL); > + if (!ilp) > + return; > + > + parent_name = of_clk_get_parent_name(np, 0); > + if (!parent_name) { > + err = -ENOENT; > + goto err_free_ilp; > + } > + > + /* TODO: This looks generic, try making it OF helper. */ > + index = of_property_match_string(np, "reg-names", "pmu"); > + if (index < 0) { > + err = index; > + goto err_free_ilp; > + } > + err = of_address_to_resource(np, index, &res); > + if (err) > + goto err_free_ilp; > + ilp->pmu = ioremap(res.start, resource_size(&res)); > + if (IS_ERR(ilp->pmu)) { > + err = PTR_ERR(ilp->pmu); > + goto err_free_ilp; > + } > + > + init.name = np->name; > + init.ops = &bcm53573_ilp_clk_ops; > + init.parent_names = &parent_name; > + init.num_parents = 1; > + > + ilp->hw.init = &init; > + ilp->clk = clk_register(NULL, &ilp->hw); please use clk_hw_register() and of_clk_add_hw_provider(). > + if (WARN_ON(IS_ERR(ilp->clk))) > + goto err_unmap_pmu; > + > + err = of_clk_add_provider(np, of_clk_src_simple_get, ilp->clk); > + if (err) > + goto err_clk_unregister; > + > + return; > + > +err_clk_unregister: > + clk_unregister(ilp->clk); > +err_unmap_pmu: > + iounmap(ilp->pmu); > +err_free_ilp: > + kfree(ilp); > + pr_err("Failed to init ILP clock: %d\n", err); > +} > +CLK_OF_DECLARE(bcm53573_ilp_clk, "brcm,bcm53573-ilp", bcm53573_ilp_init); Can this be a platform driver instead? -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Boyd Subject: Re: [PATCH V5] clk: bcm: Add driver for BCM53573 ILP clock Date: Wed, 24 Aug 2016 01:47:38 -0700 Message-ID: <20160824084738.GB6502@codeaurora.org> References: <20160823061745.8162-1-zajec5@gmail.com> <20160823062613.13865-1-zajec5@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Return-path: Content-Disposition: inline In-Reply-To: <20160823062613.13865-1-zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: =?utf-8?B?UmFmYcWCIE1pxYJlY2tp?= Cc: Michael Turquette , linux-clk-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w@public.gmane.org, =?utf-8?B?UmFmYcWCIE1pxYJlY2tp?= , Rob Herring , Mark Rutland , Eric Anholt , Jon Mason , Florian Fainelli , Stephen Warren , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , open list List-Id: devicetree@vger.kernel.org On 08/23, Rafał Miłecki wrote: > diff --git a/drivers/clk/bcm/clk-bcm53573-ilp.c b/drivers/clk/bcm/clk-bcm53573-ilp.c > new file mode 100644 > index 0000000..b7ac0eb > --- /dev/null > +++ b/drivers/clk/bcm/clk-bcm53573-ilp.c > @@ -0,0 +1,146 @@ > +/* > + * Copyright (C) 2016 Rafał Miłecki > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#include Is this include used? > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define PMU_XTAL_FREQ_RATIO 0x66c > +#define XTAL_ALP_PER_4ILP 0x00001fff > +#define XTAL_CTL_EN 0x80000000 > +#define PMU_SLOW_CLK_PERIOD 0x6dc > + > +struct bcm53573_ilp { > + struct clk *clk; > + struct clk_hw hw; > + void __iomem *pmu; > +}; > + > +static int bcm53573_ilp_enable(struct clk_hw *hw) > +{ > + struct bcm53573_ilp *ilp = container_of(hw, struct bcm53573_ilp, hw); > + > + writel(0x10199, ilp->pmu + PMU_SLOW_CLK_PERIOD); > + writel(0x10000, ilp->pmu + 0x674); Is there a name for 0x674? > + > + return 0; > +} > + > +static unsigned long bcm53573_ilp_recalc_rate(struct clk_hw *hw, > + unsigned long parent_rate) > +{ > + struct bcm53573_ilp *ilp = container_of(hw, struct bcm53573_ilp, hw); > + void __iomem *pmu = ilp->pmu; > + u32 last_val, cur_val; > + u32 sum = 0, num = 0, loop_num = 0; Should these just be plain ints? Do we care about sizes for these variables? > + u32 avg; This one too. > + > + /* Enable measurement */ > + writel(XTAL_CTL_EN, pmu + PMU_XTAL_FREQ_RATIO); > + > + /* Read initial value */ > + last_val = readl(pmu + PMU_XTAL_FREQ_RATIO) & XTAL_ALP_PER_4ILP; > + > + /* > + * At minimum we should loop for a bit to let hardware do the > + * measurement. This isn't very accurate however, so for a better > + * precision lets try getting 20 different values for and use average. > + */ > + while (num < 20) { > + cur_val = readl(pmu + PMU_XTAL_FREQ_RATIO) & XTAL_ALP_PER_4ILP; > + > + if (cur_val != last_val) { > + /* Got different value, use it */ > + sum += cur_val; > + num++; > + loop_num = 0; > + last_val = cur_val; > + } else if (++loop_num > 5000) { > + /* Same value over and over, give up */ > + sum += cur_val; > + num++; > + break; > + } Should there be a udelay() here? Or we're expected to tight loop read the hardware? If so we should throw in a cpu_relax() here to indicate tight loop. > + } > + > + /* Disable measurement to save power */ > + writel(0x0, pmu + PMU_XTAL_FREQ_RATIO); > + > + avg = sum / num; > + > + return parent_rate * 4 / avg; > +} > + > +static const struct clk_ops bcm53573_ilp_clk_ops = { > + .enable = bcm53573_ilp_enable, No disable? Or .is_enabled? > + .recalc_rate = bcm53573_ilp_recalc_rate, > +}; > + > +static void bcm53573_ilp_init(struct device_node *np) > +{ > + struct bcm53573_ilp *ilp; > + struct resource res; > + struct clk_init_data init = { 0 }; > + const char *parent_name; > + int index; > + int err; > + > + ilp = kzalloc(sizeof(*ilp), GFP_KERNEL); > + if (!ilp) > + return; > + > + parent_name = of_clk_get_parent_name(np, 0); > + if (!parent_name) { > + err = -ENOENT; > + goto err_free_ilp; > + } > + > + /* TODO: This looks generic, try making it OF helper. */ > + index = of_property_match_string(np, "reg-names", "pmu"); > + if (index < 0) { > + err = index; > + goto err_free_ilp; > + } > + err = of_address_to_resource(np, index, &res); > + if (err) > + goto err_free_ilp; > + ilp->pmu = ioremap(res.start, resource_size(&res)); > + if (IS_ERR(ilp->pmu)) { > + err = PTR_ERR(ilp->pmu); > + goto err_free_ilp; > + } > + > + init.name = np->name; > + init.ops = &bcm53573_ilp_clk_ops; > + init.parent_names = &parent_name; > + init.num_parents = 1; > + > + ilp->hw.init = &init; > + ilp->clk = clk_register(NULL, &ilp->hw); please use clk_hw_register() and of_clk_add_hw_provider(). > + if (WARN_ON(IS_ERR(ilp->clk))) > + goto err_unmap_pmu; > + > + err = of_clk_add_provider(np, of_clk_src_simple_get, ilp->clk); > + if (err) > + goto err_clk_unregister; > + > + return; > + > +err_clk_unregister: > + clk_unregister(ilp->clk); > +err_unmap_pmu: > + iounmap(ilp->pmu); > +err_free_ilp: > + kfree(ilp); > + pr_err("Failed to init ILP clock: %d\n", err); > +} > +CLK_OF_DECLARE(bcm53573_ilp_clk, "brcm,bcm53573-ilp", bcm53573_ilp_init); Can this be a platform driver instead? -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html