From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758551AbcHYNlZ (ORCPT ); Thu, 25 Aug 2016 09:41:25 -0400 Received: from mail-oi0-f67.google.com ([209.85.218.67]:35610 "EHLO mail-oi0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754441AbcHYNlW (ORCPT ); Thu, 25 Aug 2016 09:41:22 -0400 MIME-Version: 1.0 In-Reply-To: <20160824084738.GB6502@codeaurora.org> References: <20160823061745.8162-1-zajec5@gmail.com> <20160823062613.13865-1-zajec5@gmail.com> <20160824084738.GB6502@codeaurora.org> From: =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= Date: Thu, 25 Aug 2016 15:28:04 +0200 Message-ID: Subject: Re: [PATCH V5] clk: bcm: Add driver for BCM53573 ILP clock To: Stephen Boyd Cc: Michael Turquette , linux-clk@vger.kernel.org, bcm-kernel-feedback-list , =?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 Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id u7PDfVPK004634 On 24 August 2016 at 10:47, Stephen Boyd wrote: > 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? No. Good point. >> +#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. Right. >> + >> + /* 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? The beauty of working without datasheets... I'll compare initial reg state with one after enabling and see if there is sth obvious. >> + .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(). I wasn't aware of this API change, thanks. >> + 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? I guess it can. Should it? It's not clear to me when CLK_OF_DECLARE is preferred and when it's not. -- Rafał