[RESEND/PATCH v6 0/3] Add support for Qualcomm A53 CPU clock

[RESEND/PATCH v6 0/3] Add support for Qualcomm A53 CPU clock

[Georgi Djakov]

Changes since v5 (https://lkml.org/lkml/2016/2/1/407)
 * Rebase to clk-next and update according to the recent API changes.

Changes since v4 (https://lkml.org/lkml/2015/12/14/367)
 * Convert to builtin drivers as now __clk_lookup() is used

Changes since v3 (https://lkml.org/lkml/2015/8/12/585)
 * Split driver into two parts - and separate A53 PLL and
   A53 clock controller drivers.
 * Drop the safe switch hook patch. Add a clock notifier in
   the clock provider to handle switching via safe mux and
   divider configuration.

Changes since v2 (https://lkml.org/lkml/2015/7/24/526)
 * Drop gpll0_vote patch.
 * Switch to the new clk_hw_* APIs.
 * Rebase to the current clk-next.

Changes since v1 (https://lkml.org/lkml/2015/6/12/193)
 * Drop SR2 PLL patch, as it is already applied.
 * Add gpll0_vote rate propagation patch.
 * Update/rebase patches to the current clk-next.


Georgi Djakov (3):
  clk: qcom: Add A53 PLL support
  clk: qcom: Add regmap mux-div clocks support
  clk: qcom: Add A53 clock driver

 .../devicetree/bindings/clock/qcom,a53-pll.txt     |  17 ++
 .../devicetree/bindings/clock/qcom,a53cc.txt       |  22 ++
 drivers/clk/qcom/Kconfig                           |  17 ++
 drivers/clk/qcom/Makefile                          |   3 +
 drivers/clk/qcom/a53-pll.c                         |  94 ++++++++
 drivers/clk/qcom/a53cc.c                           | 155 +++++++++++++
 drivers/clk/qcom/clk-regmap-mux-div.c              | 254 +++++++++++++++++++++
 drivers/clk/qcom/clk-regmap-mux-div.h              |  58 +++++
 8 files changed, 620 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,a53-pll.txt
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,a53cc.txt
 create mode 100644 drivers/clk/qcom/a53-pll.c
 create mode 100644 drivers/clk/qcom/a53cc.c
 create mode 100644 drivers/clk/qcom/clk-regmap-mux-div.c
 create mode 100644 drivers/clk/qcom/clk-regmap-mux-div.h

[RESEND/PATCH v6 1/3] clk: qcom: Add A53 PLL support

[Georgi Djakov]

Add support for the PLL, which generates the higher range of CPU
frequencies on MSM8916 platforms.

Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
---
 .../devicetree/bindings/clock/qcom,a53-pll.txt     | 17 ++++
 drivers/clk/qcom/Kconfig                           |  9 +++
 drivers/clk/qcom/Makefile                          |  1 +
 drivers/clk/qcom/a53-pll.c                         | 94 ++++++++++++++++++++++
 4 files changed, 121 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,a53-pll.txt
 create mode 100644 drivers/clk/qcom/a53-pll.c

diff --git a/Documentation/devicetree/bindings/clock/qcom,a53-pll.txt b/Documentation/devicetree/bindings/clock/qcom,a53-pll.txt
new file mode 100644
index 000000000000..50e71e4e13a6
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/qcom,a53-pll.txt
@@ -0,0 +1,17 @@
+A53 PLL Binding
+---------------
+The A53 PLL is the main CPU PLL used for frequencies above 1GHz.
+
+Required properties :
+- compatible : Shall contain only one of the following:
+
+		"qcom,a53-pll"
+
+- reg : shall contain base register location and length
+
+Example:
+
+	a53pll: a53pll@0b016000 {
+		compatible = "qcom,a53-pll";
+		reg = <0x0b016000 0x40>;
+	};
diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index 0146d3c2547f..a889f0b14b54 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -150,3 +150,12 @@ config MSM_MMCC_8996
 	  Support for the multimedia clock controller on msm8996 devices.
 	  Say Y if you want to support multimedia devices such as display,
 	  graphics, video encode/decode, camera, etc.
+
+config QCOM_A53PLL
+	bool "A53 PLL"
+	depends on COMMON_CLK_QCOM
+	help
+	  Support for the A53 PLL on some Qualcomm devices. It provides
+	  support for CPU frequencies above 1GHz.
+	  Say Y if you want to support CPU frequency scaling on devices
+	  such as MSM8916.
diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index 1fb1f5476cb0..7d27f47f0c92 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -29,3 +29,4 @@ obj-$(CONFIG_MSM_LCC_8960) += lcc-msm8960.o
 obj-$(CONFIG_MSM_MMCC_8960) += mmcc-msm8960.o
 obj-$(CONFIG_MSM_MMCC_8974) += mmcc-msm8974.o
 obj-$(CONFIG_MSM_MMCC_8996) += mmcc-msm8996.o
+obj-$(CONFIG_QCOM_A53PLL) += a53-pll.o
diff --git a/drivers/clk/qcom/a53-pll.c b/drivers/clk/qcom/a53-pll.c
new file mode 100644
index 000000000000..43902080f34b
--- /dev/null
+++ b/drivers/clk/qcom/a53-pll.c
@@ -0,0 +1,94 @@
+/*
+ * Copyright (c) 2016, Linaro Limited
+ * Copyright (c) 2014, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#include "clk-pll.h"
+#include "clk-regmap.h"
+
+static struct pll_freq_tbl a53pll_freq[] = {
+	{  998400000, 52, 0x0, 0x1, 0 },
+	{ 1094400000, 57, 0x0, 0x1, 0 },
+	{ 1152000000, 62, 0x0, 0x1, 0 },
+	{ 1209600000, 65, 0x0, 0x1, 0 },
+	{ 1401600000, 73, 0x0, 0x1, 0 },
+};
+
+static const struct regmap_config a53pll_regmap_config = {
+	.reg_bits		= 32,
+	.reg_stride		= 4,
+	.val_bits		= 32,
+	.max_register		= 0x40,
+	.fast_io		= true,
+	.val_format_endian	= REGMAP_ENDIAN_LITTLE,
+};
+
+static const struct of_device_id qcom_a53pll_match_table[] = {
+	{ .compatible = "qcom,a53-pll" },
+	{ }
+};
+
+static int qcom_a53pll_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *node = dev->of_node;
+	struct clk_pll *pll;
+	struct resource *res;
+	void __iomem *base;
+	struct regmap *regmap;
+	struct clk_init_data init;
+
+	pll = devm_kzalloc(dev, sizeof(*pll), GFP_KERNEL);
+	if (!pll)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	regmap = devm_regmap_init_mmio(dev, base, &a53pll_regmap_config);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	pll->l_reg = 0x04,
+	pll->m_reg = 0x08,
+	pll->n_reg = 0x0c,
+	pll->config_reg = 0x14,
+	pll->mode_reg = 0x00,
+	pll->status_reg = 0x1c,
+	pll->status_bit = 16,
+	pll->freq_tbl = a53pll_freq,
+
+	init.name = node->name;
+	init.parent_names = (const char *[]){ "xo" },
+	init.num_parents = 1,
+	init.ops = &clk_pll_sr2_ops,
+	init.flags = CLK_IS_CRITICAL;
+	pll->clkr.hw.init = &init;
+
+	return devm_clk_register_regmap(dev, &pll->clkr);
+}
+
+static struct platform_driver qcom_a53pll_driver = {
+	.probe = qcom_a53pll_probe,
+	.driver = {
+		.name = "qcom-a53pll",
+		.of_match_table = qcom_a53pll_match_table,
+	},
+};
+
+builtin_platform_driver(qcom_a53pll_driver);

[RESEND/PATCH v6 2/3] clk: qcom: Add regmap mux-div clocks support

[Georgi Djakov]

Add support for hardware that can switch both parent clocks and divider
at the same time. This avoids generating intermediate frequencies from
either the old parent clock and new divider or new parent clock and
old divider combinations.

Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
---
 drivers/clk/qcom/Makefile             |   1 +
 drivers/clk/qcom/clk-regmap-mux-div.c | 254 ++++++++++++++++++++++++++++++++++
 drivers/clk/qcom/clk-regmap-mux-div.h |  58 ++++++++
 3 files changed, 313 insertions(+)
 create mode 100644 drivers/clk/qcom/clk-regmap-mux-div.c
 create mode 100644 drivers/clk/qcom/clk-regmap-mux-div.h

diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index 7d27f47f0c92..d3e142e577b0 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -9,6 +9,7 @@ clk-qcom-y += clk-rcg2.o
 clk-qcom-y += clk-branch.o
 clk-qcom-y += clk-regmap-divider.o
 clk-qcom-y += clk-regmap-mux.o
+clk-qcom-y += clk-regmap-mux-div.o
 clk-qcom-y += reset.o
 clk-qcom-$(CONFIG_QCOM_GDSC) += gdsc.o
 
diff --git a/drivers/clk/qcom/clk-regmap-mux-div.c b/drivers/clk/qcom/clk-regmap-mux-div.c
new file mode 100644
index 000000000000..ec87f496606a
--- /dev/null
+++ b/drivers/clk/qcom/clk-regmap-mux-div.c
@@ -0,0 +1,254 @@
+/*
+ * Copyright (c) 2015, Linaro Limited
+ * Copyright (c) 2014, The Linux Foundation. All rights reserved.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/bitops.h>
+#include <linux/delay.h>
+#include <linux/export.h>
+#include <linux/kernel.h>
+#include <linux/regmap.h>
+
+#include "clk-regmap-mux-div.h"
+
+#define CMD_RCGR			0x0
+#define CMD_RCGR_UPDATE			BIT(0)
+#define CMD_RCGR_DIRTY_CFG		BIT(4)
+#define CMD_RCGR_ROOT_OFF		BIT(31)
+#define CFG_RCGR			0x4
+
+#define to_clk_regmap_mux_div(_hw) \
+	container_of(to_clk_regmap(_hw), struct clk_regmap_mux_div, clkr)
+
+int __mux_div_set_src_div(struct clk_regmap_mux_div *md, u32 src, u32 div)
+{
+	int ret, count;
+	u32 val, mask;
+	const char *name = clk_hw_get_name(&md->clkr.hw);
+
+	val = (div << md->hid_shift) | (src << md->src_shift);
+	mask = ((BIT(md->hid_width) - 1) << md->hid_shift) |
+	       ((BIT(md->src_width) - 1) << md->src_shift);
+
+	ret = regmap_update_bits(md->clkr.regmap, CFG_RCGR + md->reg_offset,
+				 mask, val);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(md->clkr.regmap, CMD_RCGR + md->reg_offset,
+				 CMD_RCGR_UPDATE, CMD_RCGR_UPDATE);
+	if (ret)
+		return ret;
+
+	/* Wait for update to take effect */
+	for (count = 500; count > 0; count--) {
+		ret = regmap_read(md->clkr.regmap, CMD_RCGR + md->reg_offset,
+				  &val);
+		if (ret)
+			return ret;
+		if (!(val & CMD_RCGR_UPDATE))
+			return 0;
+		udelay(1);
+	}
+
+	pr_err("%s: RCG did not update its configuration", name);
+	return -EBUSY;
+}
+
+static void __mux_div_get_src_div(struct clk_regmap_mux_div *md, u32 *src,
+				  u32 *div)
+{
+	u32 val, __div, __src;
+	const char *name = clk_hw_get_name(&md->clkr.hw);
+
+	regmap_read(md->clkr.regmap, CMD_RCGR + md->reg_offset, &val);
+
+	if (val & CMD_RCGR_DIRTY_CFG) {
+		pr_err("%s: RCG configuration is pending\n", name);
+		return;
+	}
+
+	regmap_read(md->clkr.regmap, CFG_RCGR + md->reg_offset, &val);
+	__src = (val >> md->src_shift);
+	__src &= BIT(md->src_width) - 1;
+	*src = __src;
+
+	__div = (val >> md->hid_shift);
+	__div &= BIT(md->hid_width) - 1;
+	*div = __div;
+}
+
+static int mux_div_enable(struct clk_hw *hw)
+{
+	struct clk_regmap_mux_div *md = to_clk_regmap_mux_div(hw);
+
+	return __mux_div_set_src_div(md, md->src, md->div);
+}
+
+static inline bool is_better_rate(unsigned long req, unsigned long best,
+				  unsigned long new)
+{
+	return (req <= new && new < best) || (best < req && best < new);
+}
+
+static int mux_div_determine_rate(struct clk_hw *hw,
+				  struct clk_rate_request *req)
+{
+	struct clk_regmap_mux_div *md = to_clk_regmap_mux_div(hw);
+	unsigned int i, div, max_div;
+	unsigned long actual_rate, best_rate = 0;
+	unsigned long req_rate = req->rate;
+
+	for (i = 0; i < clk_hw_get_num_parents(hw); i++) {
+		struct clk_hw *parent = clk_hw_get_parent_by_index(hw, i);
+		unsigned long parent_rate = clk_hw_get_rate(parent);
+
+		max_div = BIT(md->hid_width) - 1;
+		for (div = 1; div < max_div; div++) {
+			parent_rate = mult_frac(req_rate, div, 2);
+			parent_rate = clk_hw_round_rate(parent, parent_rate);
+			actual_rate = mult_frac(parent_rate, 2, div);
+
+			if (is_better_rate(req_rate, best_rate, actual_rate)) {
+				best_rate = actual_rate;
+				req->rate = best_rate;
+				req->best_parent_rate = parent_rate;
+				req->best_parent_hw = parent;
+			}
+
+			if (actual_rate < req_rate || best_rate <= req_rate)
+				break;
+		}
+	}
+
+	if (!best_rate)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int __mux_div_set_rate_and_parent(struct clk_hw *hw, unsigned long rate,
+					 unsigned long prate, u32 src)
+{
+	struct clk_regmap_mux_div *md = to_clk_regmap_mux_div(hw);
+	int ret;
+	u32 div, max_div, best_src = 0, best_div = 0;
+	unsigned int i;
+	unsigned long actual_rate, best_rate = 0;
+
+	for (i = 0; i < clk_hw_get_num_parents(hw); i++) {
+		struct clk_hw *parent = clk_hw_get_parent_by_index(hw, i);
+		unsigned long parent_rate = clk_hw_get_rate(parent);
+
+		max_div = BIT(md->hid_width) - 1;
+		for (div = 1; div < max_div; div++) {
+			parent_rate = mult_frac(rate, div, 2);
+			parent_rate = clk_hw_round_rate(parent, parent_rate);
+			actual_rate = mult_frac(parent_rate, 2, div);
+
+			if (is_better_rate(rate, best_rate, actual_rate)) {
+				best_rate = actual_rate;
+				best_src = md->parent_map[i].cfg;
+				best_div = div - 1;
+			}
+
+			if (actual_rate < rate || best_rate <= rate)
+				break;
+		}
+	}
+
+	ret = __mux_div_set_src_div(md, best_src, best_div);
+	if (!ret) {
+		md->div = best_div;
+		md->src = best_src;
+	}
+
+	return ret;
+}
+
+static u8 mux_div_get_parent(struct clk_hw *hw)
+{
+	struct clk_regmap_mux_div *md = to_clk_regmap_mux_div(hw);
+	const char *name = clk_hw_get_name(hw);
+	u32 i, div, src = 0;
+
+	__mux_div_get_src_div(md, &src, &div);
+
+	for (i = 0; i < clk_hw_get_num_parents(hw); i++)
+		if (src == md->parent_map[i].cfg)
+			return i;
+
+	pr_err("%s: Can't find parent with src %d\n", name, src);
+	return 0;
+}
+
+static int mux_div_set_parent(struct clk_hw *hw, u8 index)
+{
+	struct clk_regmap_mux_div *md = to_clk_regmap_mux_div(hw);
+
+	return __mux_div_set_src_div(md, md->parent_map[index].cfg, md->div);
+}
+
+static int mux_div_set_rate(struct clk_hw *hw,
+			    unsigned long rate, unsigned long prate)
+{
+	struct clk_regmap_mux_div *md = to_clk_regmap_mux_div(hw);
+
+	return __mux_div_set_rate_and_parent(hw, rate, prate, md->src);
+}
+
+static int mux_div_set_rate_and_parent(struct clk_hw *hw,  unsigned long rate,
+				       unsigned long prate, u8 index)
+{
+	struct clk_regmap_mux_div *md = to_clk_regmap_mux_div(hw);
+
+	return __mux_div_set_rate_and_parent(hw, rate, prate,
+					     md->parent_map[index].cfg);
+}
+
+static unsigned long mux_div_recalc_rate(struct clk_hw *hw, unsigned long prate)
+{
+	struct clk_regmap_mux_div *md = to_clk_regmap_mux_div(hw);
+	u32 div, src;
+	int i, num_parents = clk_hw_get_num_parents(hw);
+	const char *name = clk_hw_get_name(hw);
+
+	__mux_div_get_src_div(md, &src, &div);
+	for (i = 0; i < num_parents; i++)
+		if (src == md->parent_map[i].cfg) {
+			struct clk_hw *p = clk_hw_get_parent_by_index(hw, i);
+			unsigned long parent_rate = clk_hw_get_rate(p);
+
+			return mult_frac(parent_rate, 2, div + 1);
+		}
+
+	pr_err("%s: Can't find parent %d\n", name, src);
+	return 0;
+}
+
+static void mux_div_disable(struct clk_hw *hw)
+{
+	struct clk_regmap_mux_div *md = to_clk_regmap_mux_div(hw);
+
+	__mux_div_set_src_div(md, md->safe_src, md->safe_div);
+}
+
+const struct clk_ops clk_regmap_mux_div_ops = {
+	.enable = mux_div_enable,
+	.disable = mux_div_disable,
+	.get_parent = mux_div_get_parent,
+	.set_parent = mux_div_set_parent,
+	.set_rate = mux_div_set_rate,
+	.set_rate_and_parent = mux_div_set_rate_and_parent,
+	.determine_rate = mux_div_determine_rate,
+	.recalc_rate = mux_div_recalc_rate,
+};
diff --git a/drivers/clk/qcom/clk-regmap-mux-div.h b/drivers/clk/qcom/clk-regmap-mux-div.h
new file mode 100644
index 000000000000..6d7ed44ae464
--- /dev/null
+++ b/drivers/clk/qcom/clk-regmap-mux-div.h
@@ -0,0 +1,58 @@
+/*
+ * Copyright (c) 2015, Linaro Limited
+ * Copyright (c) 2014, The Linux Foundation. All rights reserved.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __QCOM_CLK_REGMAP_MUX_DIV_H__
+#define __QCOM_CLK_REGMAP_MUX_DIV_H__
+
+#include <linux/clk-provider.h>
+#include "clk-rcg.h"
+#include "clk-regmap.h"
+
+/**
+ * struct mux_div_clk - combined mux/divider clock
+ * @reg_offset: offset of the mux/divider register
+ * @hid_width:	number of bits in half integer divider
+ * @hid_shift:	lowest bit of hid value field
+ * @src_width:	number of bits in source select
+ * @src_shift:	lowest bit of source select field
+ * @div:	the divider raw configuration value
+ * @src:	the mux index which will be used if the clock is enabled
+ * @safe_src:	the safe source mux value we switch to, while the main PLL is
+ *		reconfigured
+ * @safe_div:	the safe divider value that we set, while the main PLL is
+ *		reconfigured
+ * @parent_map:	pointer to parent_map struct
+ * @clkr:	handle between common and hardware-specific interfaces
+ * @clk_nb:	clock notifier registered for clock rate changes of the A53 PLL
+ */
+
+struct clk_regmap_mux_div {
+	u32				reg_offset;
+	u32				hid_width;
+	u32				hid_shift;
+	u32				src_width;
+	u32				src_shift;
+	u32				div;
+	u32				src;
+	u32				safe_src;
+	u32				safe_div;
+	const struct parent_map		*parent_map;
+	struct clk_regmap		clkr;
+	struct notifier_block		clk_nb;
+};
+
+extern const struct clk_ops clk_regmap_mux_div_ops;
+int __mux_div_set_src_div(struct clk_regmap_mux_div *md, u32 src, u32 div);
+
+#endif

[RESEND/PATCH v6 3/3] clk: qcom: Add A53 clock driver

[Georgi Djakov]

Add a driver for the A53 Clock Controller. It is a hardware block that
implements a combined mux and half integer divider functionality. It can
choose between a fixed-rate clock or the dedicated A53 PLL. The source
and the divider can be set both at the same time.

This is required for enabling CPU frequency scaling on platforms like
MSM8916.

Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
---
 .../devicetree/bindings/clock/qcom,a53cc.txt       |  22 +++
 drivers/clk/qcom/Kconfig                           |   8 ++
 drivers/clk/qcom/Makefile                          |   1 +
 drivers/clk/qcom/a53cc.c                           | 155 +++++++++++++++++++++
 4 files changed, 186 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,a53cc.txt
 create mode 100644 drivers/clk/qcom/a53cc.c

diff --git a/Documentation/devicetree/bindings/clock/qcom,a53cc.txt b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt
new file mode 100644
index 000000000000..a025f062f177
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt
@@ -0,0 +1,22 @@
+Qualcomm A53 CPU Clock Controller Binding
+------------------------------------------------
+The A53 CPU Clock Controller is hardware, which provides a combined
+mux and divider functionality for the CPU clocks. It can choose between
+a fixed rate clock and the dedicated A53 PLL.
+
+Required properties :
+- compatible : shall contain:
+
+			"qcom,a53cc"
+
+- reg : shall contain base register location and length
+	of the APCS region
+- #clock-cells : shall contain 1
+
+Example:
+
+	apcs: syscon@b011000 {
+		compatible = "qcom,a53cc", "syscon";
+		reg = <0x0b011000 0x1000>;
+		#clock-cells = <1>;
+	};
diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index a889f0b14b54..59dfcdc340e4 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -159,3 +159,11 @@ config QCOM_A53PLL
 	  support for CPU frequencies above 1GHz.
 	  Say Y if you want to support CPU frequency scaling on devices
 	  such as MSM8916.
+
+config QCOM_A53CC
+	bool "A53 Clock Controller"
+	depends on COMMON_CLK_QCOM && QCOM_A53PLL
+	help
+	  Support for the A53 clock controller on some Qualcomm devices.
+	  Say Y if you want to support CPU frequency scaling on devices
+	  such as MSM8916.
diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index d3e142e577b0..980a5d729aa4 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -30,4 +30,5 @@ obj-$(CONFIG_MSM_LCC_8960) += lcc-msm8960.o
 obj-$(CONFIG_MSM_MMCC_8960) += mmcc-msm8960.o
 obj-$(CONFIG_MSM_MMCC_8974) += mmcc-msm8974.o
 obj-$(CONFIG_MSM_MMCC_8996) += mmcc-msm8996.o
+obj-$(CONFIG_QCOM_A53CC) += a53cc.o
 obj-$(CONFIG_QCOM_A53PLL) += a53-pll.o
diff --git a/drivers/clk/qcom/a53cc.c b/drivers/clk/qcom/a53cc.c
new file mode 100644
index 000000000000..4d20db9da407
--- /dev/null
+++ b/drivers/clk/qcom/a53cc.c
@@ -0,0 +1,155 @@
+/*
+ * Copyright (c) 2016, Linaro Limited
+ * Copyright (c) 2014, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/cpu.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#include "clk-regmap.h"
+#include "clk-regmap-mux-div.h"
+
+enum {
+	P_GPLL0,
+	P_A53PLL,
+};
+
+static const struct parent_map gpll0_a53cc_map[] = {
+	{ P_GPLL0, 4 },
+	{ P_A53PLL, 5 },
+};
+
+static const char * const gpll0_a53cc[] = {
+	"gpll0_vote",
+	"a53pll",
+};
+
+static const struct regmap_config a53cc_regmap_config = {
+	.reg_bits		= 32,
+	.reg_stride		= 4,
+	.val_bits		= 32,
+	.max_register		= 0x1000,
+	.fast_io		= true,
+	.val_format_endian	= REGMAP_ENDIAN_LITTLE,
+};
+
+static const struct of_device_id qcom_a53cc_match_table[] = {
+	{ .compatible = "qcom,a53cc" },
+	{ }
+};
+
+/*
+ * We use the notifier function for switching to a temporary safe configuration
+ * (mux and divider), while the a53 pll is reconfigured.
+ */
+static int a53cc_notifier_cb(struct notifier_block *nb, unsigned long event,
+			     void *data)
+{
+	int ret = 0;
+	struct clk_regmap_mux_div *md = container_of(nb,
+						     struct clk_regmap_mux_div,
+						     clk_nb);
+
+	if (event == PRE_RATE_CHANGE)
+		ret = __mux_div_set_src_div(md, md->safe_src, md->safe_div);
+
+	return notifier_from_errno(ret);
+}
+
+static int qcom_a53cc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct clk_regmap_mux_div *a53cc;
+	struct resource *res;
+	void __iomem *base;
+	struct clk *pclk;
+	struct regmap *regmap;
+	struct clk_init_data init;
+	int ret;
+
+	a53cc = devm_kzalloc(dev, sizeof(*a53cc), GFP_KERNEL);
+	if (!a53cc)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	a53cc->reg_offset = 0x50,
+	a53cc->hid_width = 5,
+	a53cc->hid_shift = 0,
+	a53cc->src_width = 3,
+	a53cc->src_shift = 8,
+	a53cc->safe_src = 4,
+	a53cc->safe_div = 3,
+	a53cc->parent_map = gpll0_a53cc_map,
+
+	init.name = "a53mux",
+	init.parent_names = gpll0_a53cc,
+	init.num_parents = 2,
+	init.ops = &clk_regmap_mux_div_ops,
+	init.flags = CLK_SET_RATE_PARENT;
+	a53cc->clkr.hw.init = &init;
+
+	pclk = __clk_lookup(gpll0_a53cc[1]);
+	if (!pclk)
+		return -EPROBE_DEFER;
+
+	a53cc->clk_nb.notifier_call = a53cc_notifier_cb;
+	ret = clk_notifier_register(pclk, &a53cc->clk_nb);
+	if (ret) {
+		dev_err(dev, "failed to register clock notifier: %d\n", ret);
+		return ret;
+	}
+
+	regmap = devm_regmap_init_mmio(dev, base, &a53cc_regmap_config);
+	if (IS_ERR(regmap)) {
+		ret = PTR_ERR(regmap);
+		dev_err(dev, "failed to init regmap mmio: %d\n", ret);
+		goto err;
+	}
+
+	a53cc->clkr.regmap = regmap;
+
+	ret = devm_clk_register_regmap(dev, &a53cc->clkr);
+	if (ret) {
+		dev_err(dev, "failed to register regmap clock: %d\n", ret);
+		goto err;
+	}
+
+	ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_simple_get,
+				     &a53cc->clkr.hw);
+	if (ret) {
+		dev_err(dev, "failed to add clock provider: %d\n", ret);
+		goto err;
+	}
+
+	return 0;
+err:
+	clk_notifier_unregister(pclk, &a53cc->clk_nb);
+	return ret;
+}
+
+static struct platform_driver qcom_a53cc_driver = {
+	.probe = qcom_a53cc_probe,
+	.driver = {
+		.name = "qcom-a53cc",
+		.of_match_table = qcom_a53cc_match_table,
+	},
+};
+
+builtin_platform_driver(qcom_a53cc_driver);

Re: [RESEND/PATCH v6 1/3] clk: qcom: Add A53 PLL support

[Stephen Boyd]

On 10/19, Georgi Djakov wrote:
> Add support for the PLL, which generates the higher range of CPU
> frequencies on MSM8916 platforms.
> 
> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>

Please Cc dt reviewers.

> ---
>  .../devicetree/bindings/clock/qcom,a53-pll.txt     | 17 ++++
>  drivers/clk/qcom/Kconfig                           |  9 +++
>  drivers/clk/qcom/Makefile                          |  1 +
>  drivers/clk/qcom/a53-pll.c                         | 94 ++++++++++++++++++++++
>  4 files changed, 121 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/qcom,a53-pll.txt
>  create mode 100644 drivers/clk/qcom/a53-pll.c
> 
> diff --git a/Documentation/devicetree/bindings/clock/qcom,a53-pll.txt b/Documentation/devicetree/bindings/clock/qcom,a53-pll.txt
> new file mode 100644
> index 000000000000..50e71e4e13a6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/qcom,a53-pll.txt
> @@ -0,0 +1,17 @@
> +A53 PLL Binding
> +---------------
> +The A53 PLL is the main CPU PLL used for frequencies above 1GHz.

Perhaps add which SoC(s) its in?

> +
> +Required properties :
> +- compatible : Shall contain only one of the following:
> +
> +		"qcom,a53-pll"

And this could be something more SoC specific too?

> +
> +- reg : shall contain base register location and length
> +
> +Example:
> +
> +	a53pll: a53pll@0b016000 {

Drop the leading 0

> +		compatible = "qcom,a53-pll";
> +		reg = <0x0b016000 0x40>;

Should probably have #clock-cells = <0> just in case.

> +	};
> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> index 1fb1f5476cb0..7d27f47f0c92 100644
> --- a/drivers/clk/qcom/Makefile
> +++ b/drivers/clk/qcom/Makefile
> @@ -29,3 +29,4 @@ obj-$(CONFIG_MSM_LCC_8960) += lcc-msm8960.o
>  obj-$(CONFIG_MSM_MMCC_8960) += mmcc-msm8960.o
>  obj-$(CONFIG_MSM_MMCC_8974) += mmcc-msm8974.o
>  obj-$(CONFIG_MSM_MMCC_8996) += mmcc-msm8996.o
> +obj-$(CONFIG_QCOM_A53PLL) += a53-pll.o
> diff --git a/drivers/clk/qcom/a53-pll.c b/drivers/clk/qcom/a53-pll.c
> new file mode 100644
> index 000000000000..43902080f34b
> --- /dev/null
> +++ b/drivers/clk/qcom/a53-pll.c
> @@ -0,0 +1,94 @@
> +/*
> + * Copyright (c) 2016, Linaro Limited
> + * Copyright (c) 2014, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>

#include <linux/clk-provider.h>?

> +
> +#include "clk-pll.h"
> +#include "clk-regmap.h"
> +
> +static struct pll_freq_tbl a53pll_freq[] = {

Can this be const?

> +	{  998400000, 52, 0x0, 0x1, 0 },
> +	{ 1094400000, 57, 0x0, 0x1, 0 },
> +	{ 1152000000, 62, 0x0, 0x1, 0 },
> +	{ 1209600000, 65, 0x0, 0x1, 0 },
> +	{ 1401600000, 73, 0x0, 0x1, 0 },
> +};
> +
> +static const struct regmap_config a53pll_regmap_config = {
> +	.reg_bits		= 32,
> +	.reg_stride		= 4,
> +	.val_bits		= 32,
> +	.max_register		= 0x40,
> +	.fast_io		= true,
> +	.val_format_endian	= REGMAP_ENDIAN_LITTLE,
> +};
> +
> +static const struct of_device_id qcom_a53pll_match_table[] = {
> +	{ .compatible = "qcom,a53-pll" },
> +	{ }
> +};
> +
> +static int qcom_a53pll_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node = dev->of_node;
> +	struct clk_pll *pll;
> +	struct resource *res;
> +	void __iomem *base;
> +	struct regmap *regmap;
> +	struct clk_init_data init;

Initialize to { } for safety?

> +
> +	pll = devm_kzalloc(dev, sizeof(*pll), GFP_KERNEL);
> +	if (!pll)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	regmap = devm_regmap_init_mmio(dev, base, &a53pll_regmap_config);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	pll->l_reg = 0x04,
> +	pll->m_reg = 0x08,
> +	pll->n_reg = 0x0c,
> +	pll->config_reg = 0x14,
> +	pll->mode_reg = 0x00,
> +	pll->status_reg = 0x1c,
> +	pll->status_bit = 16,
> +	pll->freq_tbl = a53pll_freq,

Replace commas with semicolon please.

> +
> +	init.name = node->name;

Please just hardcode the string name for now. Best to not get
tied down to DT node names if we don't need to.

> +	init.parent_names = (const char *[]){ "xo" },
> +	init.num_parents = 1,
> +	init.ops = &clk_pll_sr2_ops,
> +	init.flags = CLK_IS_CRITICAL;
> +	pll->clkr.hw.init = &init;
> +
> +	return devm_clk_register_regmap(dev, &pll->clkr);
> +}
> +
> +static struct platform_driver qcom_a53pll_driver = {
> +	.probe = qcom_a53pll_probe,
> +	.driver = {
> +		.name = "qcom-a53pll",
> +		.of_match_table = qcom_a53pll_match_table,
> +	},
> +};
> +
> +builtin_platform_driver(qcom_a53pll_driver);

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RESEND/PATCH v6 3/3] clk: qcom: Add A53 clock driver

[Stephen Boyd]

On 10/19, Georgi Djakov wrote:
> Add a driver for the A53 Clock Controller. It is a hardware block that
> implements a combined mux and half integer divider functionality. It can
> choose between a fixed-rate clock or the dedicated A53 PLL. The source
> and the divider can be set both at the same time.
> 
> This is required for enabling CPU frequency scaling on platforms like
> MSM8916.
> 

Please Cc DT reviewers.

> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> ---
>  .../devicetree/bindings/clock/qcom,a53cc.txt       |  22 +++
>  drivers/clk/qcom/Kconfig                           |   8 ++
>  drivers/clk/qcom/Makefile                          |   1 +
>  drivers/clk/qcom/a53cc.c                           | 155 +++++++++++++++++++++
>  4 files changed, 186 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/qcom,a53cc.txt
>  create mode 100644 drivers/clk/qcom/a53cc.c
> 
> diff --git a/Documentation/devicetree/bindings/clock/qcom,a53cc.txt b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt
> new file mode 100644
> index 000000000000..a025f062f177
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt
> @@ -0,0 +1,22 @@
> +Qualcomm A53 CPU Clock Controller Binding
> +------------------------------------------------
> +The A53 CPU Clock Controller is hardware, which provides a combined
> +mux and divider functionality for the CPU clocks. It can choose between
> +a fixed rate clock and the dedicated A53 PLL.
> +
> +Required properties :
> +- compatible : shall contain:
> +
> +			"qcom,a53cc"
> +
> +- reg : shall contain base register location and length
> +	of the APCS region
> +- #clock-cells : shall contain 1
> +
> +Example:
> +
> +	apcs: syscon@b011000 {
> +		compatible = "qcom,a53cc", "syscon";

Why is it a syscon? Is that part used?

> +		reg = <0x0b011000 0x1000>;
> +		#clock-cells = <1>;
> +	};
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index a889f0b14b54..59dfcdc340e4 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -159,3 +159,11 @@ config QCOM_A53PLL
>  	  support for CPU frequencies above 1GHz.
>  	  Say Y if you want to support CPU frequency scaling on devices
>  	  such as MSM8916.
> +
> +config QCOM_A53CC
> +	bool "A53 Clock Controller"

Can't these configs be tristate? Same applies to A53PLL.

> +	depends on COMMON_CLK_QCOM && QCOM_A53PLL
> +	help
> +	  Support for the A53 clock controller on some Qualcomm devices.
> +	  Say Y if you want to support CPU frequency scaling on devices
> +	  such as MSM8916.
> diff --git a/drivers/clk/qcom/a53cc.c b/drivers/clk/qcom/a53cc.c
> new file mode 100644
> index 000000000000..4d20db9da407
> --- /dev/null
> +++ b/drivers/clk/qcom/a53cc.c
> @@ -0,0 +1,155 @@
> +/*
> + * Copyright (c) 2016, Linaro Limited
> + * Copyright (c) 2014, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/cpu.h>

Is this include used?

> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include "clk-regmap.h"
> +#include "clk-regmap-mux-div.h"
> +
> +enum {
> +	P_GPLL0,
> +	P_A53PLL,
> +};
> +
> +static const struct parent_map gpll0_a53cc_map[] = {
> +	{ P_GPLL0, 4 },
> +	{ P_A53PLL, 5 },
> +};
> +
> +static const char * const gpll0_a53cc[] = {
> +	"gpll0_vote",
> +	"a53pll",
> +};
> +
> +static const struct regmap_config a53cc_regmap_config = {
> +	.reg_bits		= 32,
> +	.reg_stride		= 4,
> +	.val_bits		= 32,
> +	.max_register		= 0x1000,
> +	.fast_io		= true,
> +	.val_format_endian	= REGMAP_ENDIAN_LITTLE,
> +};
> +
> +static const struct of_device_id qcom_a53cc_match_table[] = {
> +	{ .compatible = "qcom,a53cc" },
> +	{ }
> +};

Can you move this down next to the driver please?

> +
> +/*
> + * We use the notifier function for switching to a temporary safe configuration
> + * (mux and divider), while the a53 pll is reconfigured.
> + */
> +static int a53cc_notifier_cb(struct notifier_block *nb, unsigned long event,
> +			     void *data)
> +{
> +	int ret = 0;
> +	struct clk_regmap_mux_div *md = container_of(nb,
> +						     struct clk_regmap_mux_div,
> +						     clk_nb);
> +
> +	if (event == PRE_RATE_CHANGE)
> +		ret = __mux_div_set_src_div(md, md->safe_src, md->safe_div);
> +
> +	return notifier_from_errno(ret);
> +}
> +
> +static int qcom_a53cc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct clk_regmap_mux_div *a53cc;
> +	struct resource *res;
> +	void __iomem *base;
> +	struct clk *pclk;
> +	struct regmap *regmap;
> +	struct clk_init_data init;

 = { } for safety?

> +	int ret;
> +
> +	a53cc = devm_kzalloc(dev, sizeof(*a53cc), GFP_KERNEL);
> +	if (!a53cc)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	a53cc->reg_offset = 0x50,
> +	a53cc->hid_width = 5,
> +	a53cc->hid_shift = 0,
> +	a53cc->src_width = 3,
> +	a53cc->src_shift = 8,
> +	a53cc->safe_src = 4,
> +	a53cc->safe_div = 3,

Replace commas with semicolons please.

Also do we need the safe things to be part of the struct? The
notifier is here so we could just as easily hard code these
things in the notifier.

> +	a53cc->parent_map = gpll0_a53cc_map,
> +
> +	init.name = "a53mux",
> +	init.parent_names = gpll0_a53cc,
> +	init.num_parents = 2,
> +	init.ops = &clk_regmap_mux_div_ops,
> +	init.flags = CLK_SET_RATE_PARENT;
> +	a53cc->clkr.hw.init = &init;
> +
> +	pclk = __clk_lookup(gpll0_a53cc[1]);
> +	if (!pclk)
> +		return -EPROBE_DEFER;
> +
> +	a53cc->clk_nb.notifier_call = a53cc_notifier_cb;
> +	ret = clk_notifier_register(pclk, &a53cc->clk_nb);
> +	if (ret) {
> +		dev_err(dev, "failed to register clock notifier: %d\n", ret);
> +		return ret;
> +	}

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RESEND/PATCH v6 2/3] clk: qcom: Add regmap mux-div clocks support

[Stephen Boyd]

On 10/19, Georgi Djakov wrote:
> diff --git a/drivers/clk/qcom/clk-regmap-mux-div.c b/drivers/clk/qcom/clk-regmap-mux-div.c
> new file mode 100644
> index 000000000000..ec87f496606a
> --- /dev/null
> +++ b/drivers/clk/qcom/clk-regmap-mux-div.c
> @@ -0,0 +1,254 @@
> +/*
> + * Copyright (c) 2015, Linaro Limited
> + * Copyright (c) 2014, The Linux Foundation. All rights reserved.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/export.h>

Are symbols exported. They probably should be for modules.

> +#include <linux/kernel.h>
> +#include <linux/regmap.h>
> +
> +#include "clk-regmap-mux-div.h"
> +
> +#define CMD_RCGR			0x0
> +#define CMD_RCGR_UPDATE			BIT(0)
> +#define CMD_RCGR_DIRTY_CFG		BIT(4)
> +#define CMD_RCGR_ROOT_OFF		BIT(31)
> +#define CFG_RCGR			0x4
> +
> +#define to_clk_regmap_mux_div(_hw) \
> +	container_of(to_clk_regmap(_hw), struct clk_regmap_mux_div, clkr)
> +
> +int __mux_div_set_src_div(struct clk_regmap_mux_div *md, u32 src, u32 div)
> +{
> +	int ret, count;
> +	u32 val, mask;
> +	const char *name = clk_hw_get_name(&md->clkr.hw);
> +
> +	val = (div << md->hid_shift) | (src << md->src_shift);
> +	mask = ((BIT(md->hid_width) - 1) << md->hid_shift) |
> +	       ((BIT(md->src_width) - 1) << md->src_shift);
> +
> +	ret = regmap_update_bits(md->clkr.regmap, CFG_RCGR + md->reg_offset,
> +				 mask, val);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(md->clkr.regmap, CMD_RCGR + md->reg_offset,
> +				 CMD_RCGR_UPDATE, CMD_RCGR_UPDATE);
> +	if (ret)
> +		return ret;
> +
> +	/* Wait for update to take effect */
> +	for (count = 500; count > 0; count--) {
> +		ret = regmap_read(md->clkr.regmap, CMD_RCGR + md->reg_offset,
> +				  &val);
> +		if (ret)
> +			return ret;
> +		if (!(val & CMD_RCGR_UPDATE))
> +			return 0;
> +		udelay(1);
> +	}
> +
> +	pr_err("%s: RCG did not update its configuration", name);
> +	return -EBUSY;
> +}
> +
> +static void __mux_div_get_src_div(struct clk_regmap_mux_div *md, u32 *src,
> +				  u32 *div)
> +{
> +	u32 val, __div, __src;

Perhaps just use d and s.

> +	const char *name = clk_hw_get_name(&md->clkr.hw);
> +
> +	regmap_read(md->clkr.regmap, CMD_RCGR + md->reg_offset, &val);
> +
> +	if (val & CMD_RCGR_DIRTY_CFG) {
> +		pr_err("%s: RCG configuration is pending\n", name);
> +		return;
> +	}
> +
> +	regmap_read(md->clkr.regmap, CFG_RCGR + md->reg_offset, &val);
> +	__src = (val >> md->src_shift);
> +	__src &= BIT(md->src_width) - 1;
> +	*src = __src;
> +
> +	__div = (val >> md->hid_shift);
> +	__div &= BIT(md->hid_width) - 1;
> +	*div = __div;
> +}
> +
> +static unsigned long mux_div_recalc_rate(struct clk_hw *hw, unsigned long prate)
> +{
> +	struct clk_regmap_mux_div *md = to_clk_regmap_mux_div(hw);
> +	u32 div, src;
> +	int i, num_parents = clk_hw_get_num_parents(hw);
> +	const char *name = clk_hw_get_name(hw);
> +
> +	__mux_div_get_src_div(md, &src, &div);
> +	for (i = 0; i < num_parents; i++)
> +		if (src == md->parent_map[i].cfg) {
> +			struct clk_hw *p = clk_hw_get_parent_by_index(hw, i);
> +			unsigned long parent_rate = clk_hw_get_rate(p);
> +
> +			return mult_frac(parent_rate, 2, div + 1);
> +		}
> +
> +	pr_err("%s: Can't find parent %d\n", name, src);
> +	return 0;
> +}
> +
> +static void mux_div_disable(struct clk_hw *hw)
> +{
> +	struct clk_regmap_mux_div *md = to_clk_regmap_mux_div(hw);
> +
> +	__mux_div_set_src_div(md, md->safe_src, md->safe_div);

Ah this is to do magic tricks with cpuidle. In the android tree
they call clk_disable() on the CPU clks from the idle path and
that switches us over to the safe source and turns off the A53
PLL while the CPU is not powered. That's all sort of sneaky and
we're not doing that yet here so let's leave that until later.
Please remove the enable/disable stuff.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RESEND/PATCH v6 1/3] clk: qcom: Add A53 PLL support

[Georgi Djakov]

Hi Stephen, thanks for reviewing!

On 10/28/2016 04:49 AM, Stephen Boyd wrote:
> On 10/19, Georgi Djakov wrote:
>> Add support for the PLL, which generates the higher range of CPU
>> frequencies on MSM8916 platforms.
>>
>> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
>
> Please Cc dt reviewers.

Ok, sure!

>
>> ---
>>  .../devicetree/bindings/clock/qcom,a53-pll.txt     | 17 ++++
>>  drivers/clk/qcom/Kconfig                           |  9 +++
>>  drivers/clk/qcom/Makefile                          |  1 +
>>  drivers/clk/qcom/a53-pll.c                         | 94 ++++++++++++++++++++++
>>  4 files changed, 121 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/clock/qcom,a53-pll.txt
>>  create mode 100644 drivers/clk/qcom/a53-pll.c
>>
>> diff --git a/Documentation/devicetree/bindings/clock/qcom,a53-pll.txt b/Documentation/devicetree/bindings/clock/qcom,a53-pll.txt
>> new file mode 100644
>> index 000000000000..50e71e4e13a6
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/qcom,a53-pll.txt
>> @@ -0,0 +1,17 @@
>> +A53 PLL Binding
>> +---------------
>> +The A53 PLL is the main CPU PLL used for frequencies above 1GHz.
>
> Perhaps add which SoC(s) its in?

Ok, done!

>
>> +
>> +Required properties :
>> +- compatible : Shall contain only one of the following:
>> +
>> +		"qcom,a53-pll"
>
> And this could be something more SoC specific too?

I will make it qcom,a53pll-msm8916

>
>> +
>> +- reg : shall contain base register location and length
>> +
>> +Example:
>> +
>> +	a53pll: a53pll@0b016000 {
>
> Drop the leading 0

Ok, done!

>
>> +		compatible = "qcom,a53-pll";
>> +		reg = <0x0b016000 0x40>;
>
> Should probably have #clock-cells = <0> just in case.
>

Ok, done!

>> +	};
>> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
>> index 1fb1f5476cb0..7d27f47f0c92 100644
>> --- a/drivers/clk/qcom/Makefile
>> +++ b/drivers/clk/qcom/Makefile
>> @@ -29,3 +29,4 @@ obj-$(CONFIG_MSM_LCC_8960) += lcc-msm8960.o
>>  obj-$(CONFIG_MSM_MMCC_8960) += mmcc-msm8960.o
>>  obj-$(CONFIG_MSM_MMCC_8974) += mmcc-msm8974.o
>>  obj-$(CONFIG_MSM_MMCC_8996) += mmcc-msm8996.o
>> +obj-$(CONFIG_QCOM_A53PLL) += a53-pll.o
>> diff --git a/drivers/clk/qcom/a53-pll.c b/drivers/clk/qcom/a53-pll.c
>> new file mode 100644
>> index 000000000000..43902080f34b
>> --- /dev/null
>> +++ b/drivers/clk/qcom/a53-pll.c
>> @@ -0,0 +1,94 @@
>> +/*
>> + * Copyright (c) 2016, Linaro Limited
>> + * Copyright (c) 2014, The Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>
> #include <linux/clk-provider.h>?
>

Ok.

>> +
>> +#include "clk-pll.h"
>> +#include "clk-regmap.h"
>> +
>> +static struct pll_freq_tbl a53pll_freq[] = {
>
> Can this be const?
>

Yes, done!

>> +	{  998400000, 52, 0x0, 0x1, 0 },
>> +	{ 1094400000, 57, 0x0, 0x1, 0 },
>> +	{ 1152000000, 62, 0x0, 0x1, 0 },
>> +	{ 1209600000, 65, 0x0, 0x1, 0 },
>> +	{ 1401600000, 73, 0x0, 0x1, 0 },
>> +};
>> +
>> +static const struct regmap_config a53pll_regmap_config = {
>> +	.reg_bits		= 32,
>> +	.reg_stride		= 4,
>> +	.val_bits		= 32,
>> +	.max_register		= 0x40,
>> +	.fast_io		= true,
>> +	.val_format_endian	= REGMAP_ENDIAN_LITTLE,
>> +};
>> +
>> +static const struct of_device_id qcom_a53pll_match_table[] = {
>> +	{ .compatible = "qcom,a53-pll" },
>> +	{ }
>> +};
>> +
>> +static int qcom_a53pll_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *node = dev->of_node;
>> +	struct clk_pll *pll;
>> +	struct resource *res;
>> +	void __iomem *base;
>> +	struct regmap *regmap;
>> +	struct clk_init_data init;
>
> Initialize to { } for safety?
>

Ok!

>> +
>> +	pll = devm_kzalloc(dev, sizeof(*pll), GFP_KERNEL);
>> +	if (!pll)
>> +		return -ENOMEM;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	base = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(base))
>> +		return PTR_ERR(base);
>> +
>> +	regmap = devm_regmap_init_mmio(dev, base, &a53pll_regmap_config);
>> +	if (IS_ERR(regmap))
>> +		return PTR_ERR(regmap);
>> +
>> +	pll->l_reg = 0x04,
>> +	pll->m_reg = 0x08,
>> +	pll->n_reg = 0x0c,
>> +	pll->config_reg = 0x14,
>> +	pll->mode_reg = 0x00,
>> +	pll->status_reg = 0x1c,
>> +	pll->status_bit = 16,
>> +	pll->freq_tbl = a53pll_freq,
>
> Replace commas with semicolon please.
>

Ah, right! Done!

>> +
>> +	init.name = node->name;
>
> Please just hardcode the string name for now. Best to not get
> tied down to DT node names if we don't need to.
>

Ok, will hardcode it as a53pll.

>> +	init.parent_names = (const char *[]){ "xo" },
>> +	init.num_parents = 1,
>> +	init.ops = &clk_pll_sr2_ops,
>> +	init.flags = CLK_IS_CRITICAL;
>> +	pll->clkr.hw.init = &init;
>> +
>> +	return devm_clk_register_regmap(dev, &pll->clkr);
>> +}
>> +
>> +static struct platform_driver qcom_a53pll_driver = {
>> +	.probe = qcom_a53pll_probe,
>> +	.driver = {
>> +		.name = "qcom-a53pll",
>> +		.of_match_table = qcom_a53pll_match_table,
>> +	},
>> +};
>> +
>> +builtin_platform_driver(qcom_a53pll_driver);
>

Re: [RESEND/PATCH v6 2/3] clk: qcom: Add regmap mux-div clocks support

[Georgi Djakov]

On 10/28/2016 04:59 AM, Stephen Boyd wrote:
> On 10/19, Georgi Djakov wrote:
>> diff --git a/drivers/clk/qcom/clk-regmap-mux-div.c b/drivers/clk/qcom/clk-regmap-mux-div.c
>> new file mode 100644
>> index 000000000000..ec87f496606a
>> --- /dev/null
>> +++ b/drivers/clk/qcom/clk-regmap-mux-div.c
>> @@ -0,0 +1,254 @@
>> +/*
>> + * Copyright (c) 2015, Linaro Limited
>> + * Copyright (c) 2014, The Linux Foundation. All rights reserved.
>> + *
>> + * This software is licensed under the terms of the GNU General Public
>> + * License version 2, as published by the Free Software Foundation, and
>> + * may be copied, distributed, and modified under those terms.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/bitops.h>
>> +#include <linux/delay.h>
>> +#include <linux/export.h>
>
> Are symbols exported. They probably should be for modules.

In the next patch i am using __clk_lookup(), which is not exported, so i 
made this builtin too. Will remove <export.h> from here.

>
>> +#include <linux/kernel.h>
>> +#include <linux/regmap.h>
>> +
>> +#include "clk-regmap-mux-div.h"
>> +
>> +#define CMD_RCGR			0x0
>> +#define CMD_RCGR_UPDATE			BIT(0)
>> +#define CMD_RCGR_DIRTY_CFG		BIT(4)
>> +#define CMD_RCGR_ROOT_OFF		BIT(31)
>> +#define CFG_RCGR			0x4
>> +
>> +#define to_clk_regmap_mux_div(_hw) \
>> +	container_of(to_clk_regmap(_hw), struct clk_regmap_mux_div, clkr)
>> +
>> +int __mux_div_set_src_div(struct clk_regmap_mux_div *md, u32 src, u32 div)
>> +{
>> +	int ret, count;
>> +	u32 val, mask;
>> +	const char *name = clk_hw_get_name(&md->clkr.hw);
>> +
>> +	val = (div << md->hid_shift) | (src << md->src_shift);
>> +	mask = ((BIT(md->hid_width) - 1) << md->hid_shift) |
>> +	       ((BIT(md->src_width) - 1) << md->src_shift);
>> +
>> +	ret = regmap_update_bits(md->clkr.regmap, CFG_RCGR + md->reg_offset,
>> +				 mask, val);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_update_bits(md->clkr.regmap, CMD_RCGR + md->reg_offset,
>> +				 CMD_RCGR_UPDATE, CMD_RCGR_UPDATE);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Wait for update to take effect */
>> +	for (count = 500; count > 0; count--) {
>> +		ret = regmap_read(md->clkr.regmap, CMD_RCGR + md->reg_offset,
>> +				  &val);
>> +		if (ret)
>> +			return ret;
>> +		if (!(val & CMD_RCGR_UPDATE))
>> +			return 0;
>> +		udelay(1);
>> +	}
>> +
>> +	pr_err("%s: RCG did not update its configuration", name);
>> +	return -EBUSY;
>> +}
>> +
>> +static void __mux_div_get_src_div(struct clk_regmap_mux_div *md, u32 *src,
>> +				  u32 *div)
>> +{
>> +	u32 val, __div, __src;
>
> Perhaps just use d and s.
>

Ok.

>> +	const char *name = clk_hw_get_name(&md->clkr.hw);
>> +
>> +	regmap_read(md->clkr.regmap, CMD_RCGR + md->reg_offset, &val);
>> +
>> +	if (val & CMD_RCGR_DIRTY_CFG) {
>> +		pr_err("%s: RCG configuration is pending\n", name);
>> +		return;
>> +	}
>> +
>> +	regmap_read(md->clkr.regmap, CFG_RCGR + md->reg_offset, &val);
>> +	__src = (val >> md->src_shift);
>> +	__src &= BIT(md->src_width) - 1;
>> +	*src = __src;
>> +
>> +	__div = (val >> md->hid_shift);
>> +	__div &= BIT(md->hid_width) - 1;
>> +	*div = __div;
>> +}
>> +
>> +static unsigned long mux_div_recalc_rate(struct clk_hw *hw, unsigned long prate)
>> +{
>> +	struct clk_regmap_mux_div *md = to_clk_regmap_mux_div(hw);
>> +	u32 div, src;
>> +	int i, num_parents = clk_hw_get_num_parents(hw);
>> +	const char *name = clk_hw_get_name(hw);
>> +
>> +	__mux_div_get_src_div(md, &src, &div);
>> +	for (i = 0; i < num_parents; i++)
>> +		if (src == md->parent_map[i].cfg) {
>> +			struct clk_hw *p = clk_hw_get_parent_by_index(hw, i);
>> +			unsigned long parent_rate = clk_hw_get_rate(p);
>> +
>> +			return mult_frac(parent_rate, 2, div + 1);
>> +		}
>> +
>> +	pr_err("%s: Can't find parent %d\n", name, src);
>> +	return 0;
>> +}
>> +
>> +static void mux_div_disable(struct clk_hw *hw)
>> +{
>> +	struct clk_regmap_mux_div *md = to_clk_regmap_mux_div(hw);
>> +
>> +	__mux_div_set_src_div(md, md->safe_src, md->safe_div);
>
> Ah this is to do magic tricks with cpuidle. In the android tree
> they call clk_disable() on the CPU clks from the idle path and
> that switches us over to the safe source and turns off the A53
> PLL while the CPU is not powered. That's all sort of sneaky and
> we're not doing that yet here so let's leave that until later.
> Please remove the enable/disable stuff.
>

Ok, agree! Its not used currently. Removed.

Re: [RESEND/PATCH v6 3/3] clk: qcom: Add A53 clock driver

[Georgi Djakov]

On 10/28/2016 04:54 AM, Stephen Boyd wrote:
> On 10/19, Georgi Djakov wrote:
>> Add a driver for the A53 Clock Controller. It is a hardware block that
>> implements a combined mux and half integer divider functionality. It can
>> choose between a fixed-rate clock or the dedicated A53 PLL. The source
>> and the divider can be set both at the same time.
>>
>> This is required for enabling CPU frequency scaling on platforms like
>> MSM8916.
>>
>
> Please Cc DT reviewers.
>

Ok, will do.

>> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
>> ---
>>  .../devicetree/bindings/clock/qcom,a53cc.txt       |  22 +++
>>  drivers/clk/qcom/Kconfig                           |   8 ++
>>  drivers/clk/qcom/Makefile                          |   1 +
>>  drivers/clk/qcom/a53cc.c                           | 155 +++++++++++++++++++++
>>  4 files changed, 186 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/clock/qcom,a53cc.txt
>>  create mode 100644 drivers/clk/qcom/a53cc.c
>>
>> diff --git a/Documentation/devicetree/bindings/clock/qcom,a53cc.txt b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt
>> new file mode 100644
>> index 000000000000..a025f062f177
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt
>> @@ -0,0 +1,22 @@
>> +Qualcomm A53 CPU Clock Controller Binding
>> +------------------------------------------------
>> +The A53 CPU Clock Controller is hardware, which provides a combined
>> +mux and divider functionality for the CPU clocks. It can choose between
>> +a fixed rate clock and the dedicated A53 PLL.
>> +
>> +Required properties :
>> +- compatible : shall contain:
>> +
>> +			"qcom,a53cc"
>> +
>> +- reg : shall contain base register location and length
>> +	of the APCS region
>> +- #clock-cells : shall contain 1
>> +
>> +Example:
>> +
>> +	apcs: syscon@b011000 {
>> +		compatible = "qcom,a53cc", "syscon";
>
> Why is it a syscon? Is that part used?

It is not used in this patchset. I will remove the syscon part from the
example and will change the compatible to qcom,a53cc-msm8916, as this
also seems to be SoC specific.

>
>> +		reg = <0x0b011000 0x1000>;
>> +		#clock-cells = <1>;
>> +	};
>> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
>> index a889f0b14b54..59dfcdc340e4 100644
>> --- a/drivers/clk/qcom/Kconfig
>> +++ b/drivers/clk/qcom/Kconfig
>> @@ -159,3 +159,11 @@ config QCOM_A53PLL
>>  	  support for CPU frequencies above 1GHz.
>>  	  Say Y if you want to support CPU frequency scaling on devices
>>  	  such as MSM8916.
>> +
>> +config QCOM_A53CC
>> +	bool "A53 Clock Controller"
>
> Can't these configs be tristate? Same applies to A53PLL.
>

I am using __clk_lookup() in this patch below, which is not exported.
The A53PLL could be a tristate, but i just made them both builtins for
consistency.

>> +	depends on COMMON_CLK_QCOM && QCOM_A53PLL
>> +	help
>> +	  Support for the A53 clock controller on some Qualcomm devices.
>> +	  Say Y if you want to support CPU frequency scaling on devices
>> +	  such as MSM8916.
>> diff --git a/drivers/clk/qcom/a53cc.c b/drivers/clk/qcom/a53cc.c
>> new file mode 100644
>> index 000000000000..4d20db9da407
>> --- /dev/null
>> +++ b/drivers/clk/qcom/a53cc.c
>> @@ -0,0 +1,155 @@
>> +/*
>> + * Copyright (c) 2016, Linaro Limited
>> + * Copyright (c) 2014, The Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/cpu.h>
>
> Is this include used?

It isn't. Removed!

>
>> +#include <linux/kernel.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +
>> +#include "clk-regmap.h"
>> +#include "clk-regmap-mux-div.h"
>> +
>> +enum {
>> +	P_GPLL0,
>> +	P_A53PLL,
>> +};
>> +
>> +static const struct parent_map gpll0_a53cc_map[] = {
>> +	{ P_GPLL0, 4 },
>> +	{ P_A53PLL, 5 },
>> +};
>> +
>> +static const char * const gpll0_a53cc[] = {
>> +	"gpll0_vote",
>> +	"a53pll",
>> +};
>> +
>> +static const struct regmap_config a53cc_regmap_config = {
>> +	.reg_bits		= 32,
>> +	.reg_stride		= 4,
>> +	.val_bits		= 32,
>> +	.max_register		= 0x1000,
>> +	.fast_io		= true,
>> +	.val_format_endian	= REGMAP_ENDIAN_LITTLE,
>> +};
>> +
>> +static const struct of_device_id qcom_a53cc_match_table[] = {
>> +	{ .compatible = "qcom,a53cc" },
>> +	{ }
>> +};
>
> Can you move this down next to the driver please?
>

Ok, sure.

>> +
>> +/*
>> + * We use the notifier function for switching to a temporary safe configuration
>> + * (mux and divider), while the a53 pll is reconfigured.
>> + */
>> +static int a53cc_notifier_cb(struct notifier_block *nb, unsigned long event,
>> +			     void *data)
>> +{
>> +	int ret = 0;
>> +	struct clk_regmap_mux_div *md = container_of(nb,
>> +						     struct clk_regmap_mux_div,
>> +						     clk_nb);
>> +
>> +	if (event == PRE_RATE_CHANGE)
>> +		ret = __mux_div_set_src_div(md, md->safe_src, md->safe_div);
>> +
>> +	return notifier_from_errno(ret);
>> +}
>> +
>> +static int qcom_a53cc_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct clk_regmap_mux_div *a53cc;
>> +	struct resource *res;
>> +	void __iomem *base;
>> +	struct clk *pclk;
>> +	struct regmap *regmap;
>> +	struct clk_init_data init;
>
>  = { } for safety?
>

Yes, thanks!

>> +	int ret;
>> +
>> +	a53cc = devm_kzalloc(dev, sizeof(*a53cc), GFP_KERNEL);
>> +	if (!a53cc)
>> +		return -ENOMEM;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	base = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(base))
>> +		return PTR_ERR(base);
>> +
>> +	a53cc->reg_offset = 0x50,
>> +	a53cc->hid_width = 5,
>> +	a53cc->hid_shift = 0,
>> +	a53cc->src_width = 3,
>> +	a53cc->src_shift = 8,
>> +	a53cc->safe_src = 4,
>> +	a53cc->safe_div = 3,
>
> Replace commas with semicolons please.
>
> Also do we need the safe things to be part of the struct? The
> notifier is here so we could just as easily hard code these
> things in the notifier.
>

Ok, will hardcode it with some comment.

Thanks again for reviewing!

BR,
Georgi

>> +	a53cc->parent_map = gpll0_a53cc_map,
>> +
>> +	init.name = "a53mux",
>> +	init.parent_names = gpll0_a53cc,
>> +	init.num_parents = 2,
>> +	init.ops = &clk_regmap_mux_div_ops,
>> +	init.flags = CLK_SET_RATE_PARENT;
>> +	a53cc->clkr.hw.init = &init;
>> +
>> +	pclk = __clk_lookup(gpll0_a53cc[1]);
>> +	if (!pclk)
>> +		return -EPROBE_DEFER;
>> +
>> +	a53cc->clk_nb.notifier_call = a53cc_notifier_cb;
>> +	ret = clk_notifier_register(pclk, &a53cc->clk_nb);
>> +	if (ret) {
>> +		dev_err(dev, "failed to register clock notifier: %d\n", ret);
>> +		return ret;
>> +	}
>

Re: [RESEND/PATCH v6 3/3] clk: qcom: Add A53 clock driver

[Bjorn Andersson]

On Thu 27 Oct 18:54 PDT 2016, Stephen Boyd wrote:

> On 10/19, Georgi Djakov wrote:
> > Add a driver for the A53 Clock Controller. It is a hardware block that
> > implements a combined mux and half integer divider functionality. It can
> > choose between a fixed-rate clock or the dedicated A53 PLL. The source
> > and the divider can be set both at the same time.
> > 
> > This is required for enabling CPU frequency scaling on platforms like
> > MSM8916.
> > 
> 
> Please Cc DT reviewers.
> 
> > Signed-off-by: Georgi Djakov <georgi.djakov-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > ---
> >  .../devicetree/bindings/clock/qcom,a53cc.txt       |  22 +++
> >  drivers/clk/qcom/Kconfig                           |   8 ++
> >  drivers/clk/qcom/Makefile                          |   1 +
> >  drivers/clk/qcom/a53cc.c                           | 155 +++++++++++++++++++++
> >  4 files changed, 186 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/clock/qcom,a53cc.txt
> >  create mode 100644 drivers/clk/qcom/a53cc.c
> > 
> > diff --git a/Documentation/devicetree/bindings/clock/qcom,a53cc.txt b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt
> > new file mode 100644
> > index 000000000000..a025f062f177
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt
> > @@ -0,0 +1,22 @@
> > +Qualcomm A53 CPU Clock Controller Binding
> > +------------------------------------------------
> > +The A53 CPU Clock Controller is hardware, which provides a combined
> > +mux and divider functionality for the CPU clocks. It can choose between
> > +a fixed rate clock and the dedicated A53 PLL.
> > +
> > +Required properties :
> > +- compatible : shall contain:
> > +
> > +			"qcom,a53cc"
> > +
> > +- reg : shall contain base register location and length
> > +	of the APCS region
> > +- #clock-cells : shall contain 1
> > +
> > +Example:
> > +
> > +	apcs: syscon@b011000 {
> > +		compatible = "qcom,a53cc", "syscon";
> 
> Why is it a syscon? Is that part used?
> 

I use the register at offset 8 for interrupting the other subsystems, so
this must be available as something I can poke.

Which makes me think that this should be described as a "simple-mfd" and
"syscon" with the a53cc node as a child - grabbing the regmap of the
syscon parent, rather then ioremapping the same region again.

Regards,
Bjorn
--
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

Re: [RESEND/PATCH v6 3/3] clk: qcom: Add A53 clock driver

[Bjorn Andersson]

On Thu 27 Oct 18:54 PDT 2016, Stephen Boyd wrote:

> On 10/19, Georgi Djakov wrote:
> > Add a driver for the A53 Clock Controller. It is a hardware block that
> > implements a combined mux and half integer divider functionality. It can
> > choose between a fixed-rate clock or the dedicated A53 PLL. The source
> > and the divider can be set both at the same time.
> > 
> > This is required for enabling CPU frequency scaling on platforms like
> > MSM8916.
> > 
> 
> Please Cc DT reviewers.
> 
> > Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> > ---
> >  .../devicetree/bindings/clock/qcom,a53cc.txt       |  22 +++
> >  drivers/clk/qcom/Kconfig                           |   8 ++
> >  drivers/clk/qcom/Makefile                          |   1 +
> >  drivers/clk/qcom/a53cc.c                           | 155 +++++++++++++++++++++
> >  4 files changed, 186 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/clock/qcom,a53cc.txt
> >  create mode 100644 drivers/clk/qcom/a53cc.c
> > 
> > diff --git a/Documentation/devicetree/bindings/clock/qcom,a53cc.txt b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt
> > new file mode 100644
> > index 000000000000..a025f062f177
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt
> > @@ -0,0 +1,22 @@
> > +Qualcomm A53 CPU Clock Controller Binding
> > +------------------------------------------------
> > +The A53 CPU Clock Controller is hardware, which provides a combined
> > +mux and divider functionality for the CPU clocks. It can choose between
> > +a fixed rate clock and the dedicated A53 PLL.
> > +
> > +Required properties :
> > +- compatible : shall contain:
> > +
> > +			"qcom,a53cc"
> > +
> > +- reg : shall contain base register location and length
> > +	of the APCS region
> > +- #clock-cells : shall contain 1
> > +
> > +Example:
> > +
> > +	apcs: syscon@b011000 {
> > +		compatible = "qcom,a53cc", "syscon";
> 
> Why is it a syscon? Is that part used?
> 

I use the register at offset 8 for interrupting the other subsystems, so
this must be available as something I can poke.

Which makes me think that this should be described as a "simple-mfd" and
"syscon" with the a53cc node as a child - grabbing the regmap of the
syscon parent, rather then ioremapping the same region again.

Regards,
Bjorn

Re: [RESEND/PATCH v6 3/3] clk: qcom: Add A53 clock driver

[Stephen Boyd]

On 11/02, Bjorn Andersson wrote:
> On Thu 27 Oct 18:54 PDT 2016, Stephen Boyd wrote:
> 
> > On 10/19, Georgi Djakov wrote:
> > > Add a driver for the A53 Clock Controller. It is a hardware block that
> > > implements a combined mux and half integer divider functionality. It can
> > > choose between a fixed-rate clock or the dedicated A53 PLL. The source
> > > and the divider can be set both at the same time.
> > > 
> > > This is required for enabling CPU frequency scaling on platforms like
> > > MSM8916.
> > > 
> > 
> > Please Cc DT reviewers.
> > 
> > > Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> > > ---
> > >  .../devicetree/bindings/clock/qcom,a53cc.txt       |  22 +++
> > >  drivers/clk/qcom/Kconfig                           |   8 ++
> > >  drivers/clk/qcom/Makefile                          |   1 +
> > >  drivers/clk/qcom/a53cc.c                           | 155 +++++++++++++++++++++
> > >  4 files changed, 186 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/clock/qcom,a53cc.txt
> > >  create mode 100644 drivers/clk/qcom/a53cc.c
> > > 
> > > diff --git a/Documentation/devicetree/bindings/clock/qcom,a53cc.txt b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt
> > > new file mode 100644
> > > index 000000000000..a025f062f177
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt
> > > @@ -0,0 +1,22 @@
> > > +Qualcomm A53 CPU Clock Controller Binding
> > > +------------------------------------------------
> > > +The A53 CPU Clock Controller is hardware, which provides a combined
> > > +mux and divider functionality for the CPU clocks. It can choose between
> > > +a fixed rate clock and the dedicated A53 PLL.
> > > +
> > > +Required properties :
> > > +- compatible : shall contain:
> > > +
> > > +			"qcom,a53cc"
> > > +
> > > +- reg : shall contain base register location and length
> > > +	of the APCS region
> > > +- #clock-cells : shall contain 1
> > > +
> > > +Example:
> > > +
> > > +	apcs: syscon@b011000 {
> > > +		compatible = "qcom,a53cc", "syscon";
> > 
> > Why is it a syscon? Is that part used?
> > 
> 
> I use the register at offset 8 for interrupting the other subsystems, so
> this must be available as something I can poke.
> 
> Which makes me think that this should be described as a "simple-mfd" and
> "syscon" with the a53cc node as a child - grabbing the regmap of the
> syscon parent, rather then ioremapping the same region again.
> 

That's sort of a question for DT reviewers. The register space
certainly seems like a free for all with a tilt toward power
management of the CPU, similar to how this was done on Krait
based designs.

I wonder why we didn't make up some provider/consumer binding for
the "kicking" feature used by SMD/RPM code. Then this could be a
clock provider and a "kick" provider (haha #kick-cells) and the
usage of syscon/regmap wouldn't be mandatory.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RESEND/PATCH v6 3/3] clk: qcom: Add A53 clock driver

[Bjorn Andersson]

On Wed 02 Nov 15:55 PDT 2016, Stephen Boyd wrote:

> On 11/02, Bjorn Andersson wrote:
> > On Thu 27 Oct 18:54 PDT 2016, Stephen Boyd wrote:
> > 
> > > On 10/19, Georgi Djakov wrote:
> > > > Add a driver for the A53 Clock Controller. It is a hardware block that
> > > > implements a combined mux and half integer divider functionality. It can
> > > > choose between a fixed-rate clock or the dedicated A53 PLL. The source
> > > > and the divider can be set both at the same time.
> > > > 
> > > > This is required for enabling CPU frequency scaling on platforms like
> > > > MSM8916.
> > > > 
> > > 
> > > Please Cc DT reviewers.
> > > 
> > > > Signed-off-by: Georgi Djakov <georgi.djakov-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > > > ---
> > > >  .../devicetree/bindings/clock/qcom,a53cc.txt       |  22 +++
> > > >  drivers/clk/qcom/Kconfig                           |   8 ++
> > > >  drivers/clk/qcom/Makefile                          |   1 +
> > > >  drivers/clk/qcom/a53cc.c                           | 155 +++++++++++++++++++++
> > > >  4 files changed, 186 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/clock/qcom,a53cc.txt
> > > >  create mode 100644 drivers/clk/qcom/a53cc.c
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/clock/qcom,a53cc.txt b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt
> > > > new file mode 100644
> > > > index 000000000000..a025f062f177
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt
> > > > @@ -0,0 +1,22 @@
> > > > +Qualcomm A53 CPU Clock Controller Binding
> > > > +------------------------------------------------
> > > > +The A53 CPU Clock Controller is hardware, which provides a combined
> > > > +mux and divider functionality for the CPU clocks. It can choose between
> > > > +a fixed rate clock and the dedicated A53 PLL.
> > > > +
> > > > +Required properties :
> > > > +- compatible : shall contain:
> > > > +
> > > > +			"qcom,a53cc"
> > > > +
> > > > +- reg : shall contain base register location and length
> > > > +	of the APCS region
> > > > +- #clock-cells : shall contain 1
> > > > +
> > > > +Example:
> > > > +
> > > > +	apcs: syscon@b011000 {
> > > > +		compatible = "qcom,a53cc", "syscon";
> > > 
> > > Why is it a syscon? Is that part used?
> > > 
> > 
> > I use the register at offset 8 for interrupting the other subsystems, so
> > this must be available as something I can poke.
> > 
> > Which makes me think that this should be described as a "simple-mfd" and
> > "syscon" with the a53cc node as a child - grabbing the regmap of the
> > syscon parent, rather then ioremapping the same region again.
> > 
> 
> That's sort of a question for DT reviewers. The register space
> certainly seems like a free for all with a tilt toward power
> management of the CPU, similar to how this was done on Krait
> based designs.
> 

Right. But this kind of mashup blocks was the reason why simple-mfd was
put in place.

> I wonder why we didn't make up some provider/consumer binding for
> the "kicking" feature used by SMD/RPM code. Then this could be a
> clock provider and a "kick" provider (haha #kick-cells) and the
> usage of syscon/regmap wouldn't be mandatory.
> 

I did consider doing that, but had enough dependencies to put in place
as it was.

I'm in favour of us inventing a kicker API and it's found outside out
use cases as well (e.g. virtio/rpmsg).

Regards,
Bjorn
--
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

Re: [RESEND/PATCH v6 3/3] clk: qcom: Add A53 clock driver

[Bjorn Andersson]

On Wed 02 Nov 15:55 PDT 2016, Stephen Boyd wrote:

> On 11/02, Bjorn Andersson wrote:
> > On Thu 27 Oct 18:54 PDT 2016, Stephen Boyd wrote:
> > 
> > > On 10/19, Georgi Djakov wrote:
> > > > Add a driver for the A53 Clock Controller. It is a hardware block that
> > > > implements a combined mux and half integer divider functionality. It can
> > > > choose between a fixed-rate clock or the dedicated A53 PLL. The source
> > > > and the divider can be set both at the same time.
> > > > 
> > > > This is required for enabling CPU frequency scaling on platforms like
> > > > MSM8916.
> > > > 
> > > 
> > > Please Cc DT reviewers.
> > > 
> > > > Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> > > > ---
> > > >  .../devicetree/bindings/clock/qcom,a53cc.txt       |  22 +++
> > > >  drivers/clk/qcom/Kconfig                           |   8 ++
> > > >  drivers/clk/qcom/Makefile                          |   1 +
> > > >  drivers/clk/qcom/a53cc.c                           | 155 +++++++++++++++++++++
> > > >  4 files changed, 186 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/clock/qcom,a53cc.txt
> > > >  create mode 100644 drivers/clk/qcom/a53cc.c
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/clock/qcom,a53cc.txt b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt
> > > > new file mode 100644
> > > > index 000000000000..a025f062f177
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt
> > > > @@ -0,0 +1,22 @@
> > > > +Qualcomm A53 CPU Clock Controller Binding
> > > > +------------------------------------------------
> > > > +The A53 CPU Clock Controller is hardware, which provides a combined
> > > > +mux and divider functionality for the CPU clocks. It can choose between
> > > > +a fixed rate clock and the dedicated A53 PLL.
> > > > +
> > > > +Required properties :
> > > > +- compatible : shall contain:
> > > > +
> > > > +			"qcom,a53cc"
> > > > +
> > > > +- reg : shall contain base register location and length
> > > > +	of the APCS region
> > > > +- #clock-cells : shall contain 1
> > > > +
> > > > +Example:
> > > > +
> > > > +	apcs: syscon@b011000 {
> > > > +		compatible = "qcom,a53cc", "syscon";
> > > 
> > > Why is it a syscon? Is that part used?
> > > 
> > 
> > I use the register at offset 8 for interrupting the other subsystems, so
> > this must be available as something I can poke.
> > 
> > Which makes me think that this should be described as a "simple-mfd" and
> > "syscon" with the a53cc node as a child - grabbing the regmap of the
> > syscon parent, rather then ioremapping the same region again.
> > 
> 
> That's sort of a question for DT reviewers. The register space
> certainly seems like a free for all with a tilt toward power
> management of the CPU, similar to how this was done on Krait
> based designs.
> 

Right. But this kind of mashup blocks was the reason why simple-mfd was
put in place.

> I wonder why we didn't make up some provider/consumer binding for
> the "kicking" feature used by SMD/RPM code. Then this could be a
> clock provider and a "kick" provider (haha #kick-cells) and the
> usage of syscon/regmap wouldn't be mandatory.
> 

I did consider doing that, but had enough dependencies to put in place
as it was.

I'm in favour of us inventing a kicker API and it's found outside out
use cases as well (e.g. virtio/rpmsg).

Regards,
Bjorn

Re: [RESEND/PATCH v6 3/3] clk: qcom: Add A53 clock driver

[Georgi Djakov]

On 11/03/2016 08:28 PM, Bjorn Andersson wrote:
> On Wed 02 Nov 15:55 PDT 2016, Stephen Boyd wrote:
>
>> On 11/02, Bjorn Andersson wrote:
>>> On Thu 27 Oct 18:54 PDT 2016, Stephen Boyd wrote:
>>>
>>>> On 10/19, Georgi Djakov wrote:
>>>>> Add a driver for the A53 Clock Controller. It is a hardware block that
>>>>> implements a combined mux and half integer divider functionality. It can
>>>>> choose between a fixed-rate clock or the dedicated A53 PLL. The source
>>>>> and the divider can be set both at the same time.
>>>>>
>>>>> This is required for enabling CPU frequency scaling on platforms like
>>>>> MSM8916.
>>>>>
>>>>
>>>> Please Cc DT reviewers.
>>>>
>>>>> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
>>>>> ---
>>>>>  .../devicetree/bindings/clock/qcom,a53cc.txt       |  22 +++
>>>>>  drivers/clk/qcom/Kconfig                           |   8 ++
>>>>>  drivers/clk/qcom/Makefile                          |   1 +
>>>>>  drivers/clk/qcom/a53cc.c                           | 155 +++++++++++++++++++++
>>>>>  4 files changed, 186 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/clock/qcom,a53cc.txt
>>>>>  create mode 100644 drivers/clk/qcom/a53cc.c
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/clock/qcom,a53cc.txt b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt
>>>>> new file mode 100644
>>>>> index 000000000000..a025f062f177
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt
>>>>> @@ -0,0 +1,22 @@
>>>>> +Qualcomm A53 CPU Clock Controller Binding
>>>>> +------------------------------------------------
>>>>> +The A53 CPU Clock Controller is hardware, which provides a combined
>>>>> +mux and divider functionality for the CPU clocks. It can choose between
>>>>> +a fixed rate clock and the dedicated A53 PLL.
>>>>> +
>>>>> +Required properties :
>>>>> +- compatible : shall contain:
>>>>> +
>>>>> +			"qcom,a53cc"
>>>>> +
>>>>> +- reg : shall contain base register location and length
>>>>> +	of the APCS region
>>>>> +- #clock-cells : shall contain 1
>>>>> +
>>>>> +Example:
>>>>> +
>>>>> +	apcs: syscon@b011000 {
>>>>> +		compatible = "qcom,a53cc", "syscon";
>>>>
>>>> Why is it a syscon? Is that part used?
>>>>
>>>
>>> I use the register at offset 8 for interrupting the other subsystems, so
>>> this must be available as something I can poke.
>>>
>>> Which makes me think that this should be described as a "simple-mfd" and
>>> "syscon" with the a53cc node as a child - grabbing the regmap of the
>>> syscon parent, rather then ioremapping the same region again.
>>>
>>
>> That's sort of a question for DT reviewers. The register space
>> certainly seems like a free for all with a tilt toward power
>> management of the CPU, similar to how this was done on Krait
>> based designs.
>>
>
> Right. But this kind of mashup blocks was the reason why simple-mfd was
> put in place.
>

Ok, thanks for the comments. Then i will make it look like this:

	apcs: syscon@b011000 {
		compatible = "syscon", "simple-mfd";
		reg = <0x0b011000 0x1000>;

		a53mux: clock {
			compatible = "qcom,msm8916-a53cc";
			#clock-cells = <1>;
		};
	};

Thanks,
Georgi

>> I wonder why we didn't make up some provider/consumer binding for
>> the "kicking" feature used by SMD/RPM code. Then this could be a
>> clock provider and a "kick" provider (haha #kick-cells) and the
>> usage of syscon/regmap wouldn't be mandatory.
>>
>
> I did consider doing that, but had enough dependencies to put in place
> as it was.
>
> I'm in favour of us inventing a kicker API and it's found outside out
> use cases as well (e.g. virtio/rpmsg).
>
> Regards,
> Bjorn
>

Re: [RESEND/PATCH v6 3/3] clk: qcom: Add A53 clock driver

[Stephen Boyd]

On 11/11, Georgi Djakov wrote:
> On 11/03/2016 08:28 PM, Bjorn Andersson wrote:
> >On Wed 02 Nov 15:55 PDT 2016, Stephen Boyd wrote:
> >
> >>On 11/02, Bjorn Andersson wrote:
> >>>On Thu 27 Oct 18:54 PDT 2016, Stephen Boyd wrote:
> >>>
> >>>>On 10/19, Georgi Djakov wrote:
> >>>>>Add a driver for the A53 Clock Controller. It is a hardware block that
> >>>>>implements a combined mux and half integer divider functionality. It can
> >>>>>choose between a fixed-rate clock or the dedicated A53 PLL. The source
> >>>>>and the divider can be set both at the same time.
> >>>>>
> >>>>>This is required for enabling CPU frequency scaling on platforms like
> >>>>>MSM8916.
> >>>>>
> >>>>
> >>>>Please Cc DT reviewers.
> >>>>
> >>>>>Signed-off-by: Georgi Djakov <georgi.djakov-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >>>>>---
> >>>>> .../devicetree/bindings/clock/qcom,a53cc.txt       |  22 +++
> >>>>> drivers/clk/qcom/Kconfig                           |   8 ++
> >>>>> drivers/clk/qcom/Makefile                          |   1 +
> >>>>> drivers/clk/qcom/a53cc.c                           | 155 +++++++++++++++++++++
> >>>>> 4 files changed, 186 insertions(+)
> >>>>> create mode 100644 Documentation/devicetree/bindings/clock/qcom,a53cc.txt
> >>>>> create mode 100644 drivers/clk/qcom/a53cc.c
> >>>>>
> >>>>>diff --git a/Documentation/devicetree/bindings/clock/qcom,a53cc.txt b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt
> >>>>>new file mode 100644
> >>>>>index 000000000000..a025f062f177
> >>>>>--- /dev/null
> >>>>>+++ b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt
> >>>>>@@ -0,0 +1,22 @@
> >>>>>+Qualcomm A53 CPU Clock Controller Binding
> >>>>>+------------------------------------------------
> >>>>>+The A53 CPU Clock Controller is hardware, which provides a combined
> >>>>>+mux and divider functionality for the CPU clocks. It can choose between
> >>>>>+a fixed rate clock and the dedicated A53 PLL.
> >>>>>+
> >>>>>+Required properties :
> >>>>>+- compatible : shall contain:
> >>>>>+
> >>>>>+			"qcom,a53cc"
> >>>>>+
> >>>>>+- reg : shall contain base register location and length
> >>>>>+	of the APCS region
> >>>>>+- #clock-cells : shall contain 1
> >>>>>+
> >>>>>+Example:
> >>>>>+
> >>>>>+	apcs: syscon@b011000 {
> >>>>>+		compatible = "qcom,a53cc", "syscon";
> >>>>
> >>>>Why is it a syscon? Is that part used?
> >>>>
> >>>
> >>>I use the register at offset 8 for interrupting the other subsystems, so
> >>>this must be available as something I can poke.
> >>>
> >>>Which makes me think that this should be described as a "simple-mfd" and
> >>>"syscon" with the a53cc node as a child - grabbing the regmap of the
> >>>syscon parent, rather then ioremapping the same region again.
> >>>
> >>
> >>That's sort of a question for DT reviewers. The register space
> >>certainly seems like a free for all with a tilt toward power
> >>management of the CPU, similar to how this was done on Krait
> >>based designs.
> >>
> >
> >Right. But this kind of mashup blocks was the reason why simple-mfd was
> >put in place.
> >
> 
> Ok, thanks for the comments. Then i will make it look like this:
> 
> 	apcs: syscon@b011000 {
> 		compatible = "syscon", "simple-mfd";
> 		reg = <0x0b011000 0x1000>;
> 
> 		a53mux: clock {
> 			compatible = "qcom,msm8916-a53cc";
> 			#clock-cells = <1>;
> 		};
> 	};
> 
> Thanks,
> Georgi
> 
> >>I wonder why we didn't make up some provider/consumer binding for
> >>the "kicking" feature used by SMD/RPM code. Then this could be a
> >>clock provider and a "kick" provider (haha #kick-cells) and the
> >>usage of syscon/regmap wouldn't be mandatory.
> >>
> >
> >I did consider doing that, but had enough dependencies to put in place
> >as it was.
> >
> >I'm in favour of us inventing a kicker API and it's found outside out
> >use cases as well (e.g. virtio/rpmsg).
> >

I'd rather we did this kicker API as well. That way we don't need
to make a syscon and a simple-mfd to get software to work
properly. Don't other silicon vendors need a kicker API as well?
How are they kicking remote processors in other places? GPIOs?

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

Re: [RESEND/PATCH v6 3/3] clk: qcom: Add A53 clock driver

[Stephen Boyd]

On 11/11, Georgi Djakov wrote:
> On 11/03/2016 08:28 PM, Bjorn Andersson wrote:
> >On Wed 02 Nov 15:55 PDT 2016, Stephen Boyd wrote:
> >
> >>On 11/02, Bjorn Andersson wrote:
> >>>On Thu 27 Oct 18:54 PDT 2016, Stephen Boyd wrote:
> >>>
> >>>>On 10/19, Georgi Djakov wrote:
> >>>>>Add a driver for the A53 Clock Controller. It is a hardware block that
> >>>>>implements a combined mux and half integer divider functionality. It can
> >>>>>choose between a fixed-rate clock or the dedicated A53 PLL. The source
> >>>>>and the divider can be set both at the same time.
> >>>>>
> >>>>>This is required for enabling CPU frequency scaling on platforms like
> >>>>>MSM8916.
> >>>>>
> >>>>
> >>>>Please Cc DT reviewers.
> >>>>
> >>>>>Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> >>>>>---
> >>>>> .../devicetree/bindings/clock/qcom,a53cc.txt       |  22 +++
> >>>>> drivers/clk/qcom/Kconfig                           |   8 ++
> >>>>> drivers/clk/qcom/Makefile                          |   1 +
> >>>>> drivers/clk/qcom/a53cc.c                           | 155 +++++++++++++++++++++
> >>>>> 4 files changed, 186 insertions(+)
> >>>>> create mode 100644 Documentation/devicetree/bindings/clock/qcom,a53cc.txt
> >>>>> create mode 100644 drivers/clk/qcom/a53cc.c
> >>>>>
> >>>>>diff --git a/Documentation/devicetree/bindings/clock/qcom,a53cc.txt b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt
> >>>>>new file mode 100644
> >>>>>index 000000000000..a025f062f177
> >>>>>--- /dev/null
> >>>>>+++ b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt
> >>>>>@@ -0,0 +1,22 @@
> >>>>>+Qualcomm A53 CPU Clock Controller Binding
> >>>>>+------------------------------------------------
> >>>>>+The A53 CPU Clock Controller is hardware, which provides a combined
> >>>>>+mux and divider functionality for the CPU clocks. It can choose between
> >>>>>+a fixed rate clock and the dedicated A53 PLL.
> >>>>>+
> >>>>>+Required properties :
> >>>>>+- compatible : shall contain:
> >>>>>+
> >>>>>+			"qcom,a53cc"
> >>>>>+
> >>>>>+- reg : shall contain base register location and length
> >>>>>+	of the APCS region
> >>>>>+- #clock-cells : shall contain 1
> >>>>>+
> >>>>>+Example:
> >>>>>+
> >>>>>+	apcs: syscon@b011000 {
> >>>>>+		compatible = "qcom,a53cc", "syscon";
> >>>>
> >>>>Why is it a syscon? Is that part used?
> >>>>
> >>>
> >>>I use the register at offset 8 for interrupting the other subsystems, so
> >>>this must be available as something I can poke.
> >>>
> >>>Which makes me think that this should be described as a "simple-mfd" and
> >>>"syscon" with the a53cc node as a child - grabbing the regmap of the
> >>>syscon parent, rather then ioremapping the same region again.
> >>>
> >>
> >>That's sort of a question for DT reviewers. The register space
> >>certainly seems like a free for all with a tilt toward power
> >>management of the CPU, similar to how this was done on Krait
> >>based designs.
> >>
> >
> >Right. But this kind of mashup blocks was the reason why simple-mfd was
> >put in place.
> >
> 
> Ok, thanks for the comments. Then i will make it look like this:
> 
> 	apcs: syscon@b011000 {
> 		compatible = "syscon", "simple-mfd";
> 		reg = <0x0b011000 0x1000>;
> 
> 		a53mux: clock {
> 			compatible = "qcom,msm8916-a53cc";
> 			#clock-cells = <1>;
> 		};
> 	};
> 
> Thanks,
> Georgi
> 
> >>I wonder why we didn't make up some provider/consumer binding for
> >>the "kicking" feature used by SMD/RPM code. Then this could be a
> >>clock provider and a "kick" provider (haha #kick-cells) and the
> >>usage of syscon/regmap wouldn't be mandatory.
> >>
> >
> >I did consider doing that, but had enough dependencies to put in place
> >as it was.
> >
> >I'm in favour of us inventing a kicker API and it's found outside out
> >use cases as well (e.g. virtio/rpmsg).
> >

I'd rather we did this kicker API as well. That way we don't need
to make a syscon and a simple-mfd to get software to work
properly. Don't other silicon vendors need a kicker API as well?
How are they kicking remote processors in other places? GPIOs?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RESEND/PATCH v6 3/3] clk: qcom: Add A53 clock driver

[Bjorn Andersson]

On Mon 14 Nov 14:21 PST 2016, Stephen Boyd wrote:

> On 11/11, Georgi Djakov wrote:
> > On 11/03/2016 08:28 PM, Bjorn Andersson wrote:
[..]
> > >I'm in favour of us inventing a kicker API and it's found outside out
> > >use cases as well (e.g. virtio/rpmsg).
> > >
> 
> I'd rather we did this kicker API as well. That way we don't need
> to make a syscon and a simple-mfd to get software to work
> properly. Don't other silicon vendors need a kicker API as well?
> How are they kicking remote processors in other places? GPIOs?
> 

In remoteproc I have two of these:
1) da8xx_remoteproc ioremaps a register and writes a bit in it (looks
   similar to the downstream Qualcomm way)

2) omap_remoteproc acquires a mbox channel, in which it writes a
   virtqueue id to kick the remote.

So one of the two cases could have used such mechanism.

We could write up a Qualcomm specific "kicker" and probe the mailing
list regarding the interest in making that generic (i.e. changing the
names in the API and DT binding).

The sucky part is that I believe we have most of our kickers in place
already so rpm, smd, smp2p, smsm etc would all need to support both
mechanisms.

Regards,
Bjorn

Re: [RESEND/PATCH v6 3/3] clk: qcom: Add A53 clock driver

[Georgi Djakov]

On 12/05/2016 11:26 PM, Bjorn Andersson wrote:
> On Mon 14 Nov 14:21 PST 2016, Stephen Boyd wrote:
>
>> On 11/11, Georgi Djakov wrote:
>>> On 11/03/2016 08:28 PM, Bjorn Andersson wrote:
> [..]
>>>> I'm in favour of us inventing a kicker API and it's found outside out
>>>> use cases as well (e.g. virtio/rpmsg).
>>>>
>>
>> I'd rather we did this kicker API as well. That way we don't need
>> to make a syscon and a simple-mfd to get software to work
>> properly. Don't other silicon vendors need a kicker API as well?
>> How are they kicking remote processors in other places? GPIOs?
>>
>
> In remoteproc I have two of these:
> 1) da8xx_remoteproc ioremaps a register and writes a bit in it (looks
>    similar to the downstream Qualcomm way)
>
> 2) omap_remoteproc acquires a mbox channel, in which it writes a
>    virtqueue id to kick the remote.
>
> So one of the two cases could have used such mechanism.
>

I also see the potential users of such API mostly in the 
remoteproc/rpmgs subsystems.

> We could write up a Qualcomm specific "kicker" and probe the mailing
> list regarding the interest in making that generic (i.e. changing the
> names in the API and DT binding).

Yes, i am planing to do this.

>
> The sucky part is that I believe we have most of our kickers in place
> already so rpm, smd, smp2p, smsm etc would all need to support both
> mechanisms.

Agree.. we have to keep compatibility with existing dtbs.

Thanks,
Georgi