On 24 August 2016 at 10:47, Stephen Boyd [off-list ref] wrote:

On 08/23, Rafał Miłecki wrote:

quoted

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 <rafal@milecki.pl>
+ *
+ * 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 <linux/clk.h>

Is this include used?

No. Good point.

quoted

+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/slab.h>
+
+#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?

quoted

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

quoted

+     u32 avg;

This one too.

Right.

quoted

+
+     /* 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.

quoted

+     }
+
+     /* 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.

quoted

+     .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.

quoted

+     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ł