[PATCH v3 0/5] clk: hisilicon: support stub clock

[PATCH v3 0/5] clk: hisilicon: support stub clock

[Leo Yan]

This series adds support for hisilicon stub clock driver. On hi6220,
the bootloader needs load the firmware image and set info for OPPs;
after run into kernel, the stub clock driver is used to communicate
w/t firmware for cpu dynamic frequency scaling.

In patch series v1/v2, the stub clock driver simply writes request in
sram and send ipc to firmware; For patch series v3, the firmware has
been upgraded to use mailbox, so stub clock driver will call standard
mailbox APIs to request mailbox channel and send message to firmware.

These patches have been tested on 96board hikey and is used by cpufreq
driver.

Changes from v2:
* Use platform_device's probe for clock's initialization
* Use mailbox channel for clock frequency change

Changes from v1:
* Refine parameter checking for init flow
* Remove unnecessary debugging info
* Modify to check spinlock pointer when register clocks


Leo Yan (5):
  clk: hisi: refine parameter checking for init
  dt-bindings: arm: Hi6220: add doc for SRAM controller
  dt-bindings: clk: Hi6220: Document stub clock driver
  clk: Hi6220: add stub clock driver
  arm64: dts: add Hi6220's stub clock node

 .../bindings/arm/hisilicon/hisilicon.txt           |  18 ++
 .../devicetree/bindings/clock/hi6220-clock.txt     |  19 +-
 arch/arm64/boot/dts/hisilicon/hi6220.dtsi          |  23 ++
 drivers/clk/hisilicon/Kconfig                      |   2 +-
 drivers/clk/hisilicon/Makefile                     |   2 +-
 drivers/clk/hisilicon/clk-hi6220-stub.c            | 279 +++++++++++++++++++++
 drivers/clk/hisilicon/clk.c                        |  11 +-
 7 files changed, 343 insertions(+), 11 deletions(-)
 create mode 100644 drivers/clk/hisilicon/clk-hi6220-stub.c

-- 
1.9.1

[PATCH v3 1/5] clk: hisi: refine parameter checking for init

[Leo Yan]

*of_iomap()* will check the device node pointer, and if the pointer is
NULL it will return error code. So refine clock's init flow by checking
the device node with this simple way; and polish a little for the print
out message.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 drivers/clk/hisilicon/clk.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/hisilicon/clk.c b/drivers/clk/hisilicon/clk.c
index c90a897..156435d 100644
--- a/drivers/clk/hisilicon/clk.c
+++ b/drivers/clk/hisilicon/clk.c
@@ -45,14 +45,9 @@ struct hisi_clock_data __init *hisi_clk_init(struct device_node *np,
 	struct clk **clk_table;
 	void __iomem *base;
 
-	if (np) {
-		base = of_iomap(np, 0);
-		if (!base) {
-			pr_err("failed to map Hisilicon clock registers\n");
-			goto err;
-		}
-	} else {
-		pr_err("failed to find Hisilicon clock node in DTS\n");
+	base = of_iomap(np, 0);
+	if (!base) {
+		pr_err("%s: failed to map clock registers\n", __func__);
 		goto err;
 	}
 
-- 
1.9.1

[PATCH v3 2/5] dt-bindings: arm: Hi6220: add doc for SRAM controller

[Leo Yan]

Document "hisilicon,hi6220-sramctrl" for SRAM controller.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 .../devicetree/bindings/arm/hisilicon/hisilicon.txt    | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt
index c431c67..c733e28 100644
--- a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt
+++ b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt
@@ -127,6 +127,24 @@ Example:
 		#clock-cells = <1>;
 	};
 
+
+Hisilicon Hi6220 SRAM controller
+
+Required properties:
+- compatible : "hisilicon,hi6220-sramctrl", "syscon"
+- reg : Register address and size
+
+Hisilicon's SoCs use sram for multiple purpose; on Hi6220 there have several
+SRAM banks for power management, modem, security, etc. Further, use "syscon"
+managing the common sram which can be shared by multiple modules.
+
+Example:
+	/*for Hi6220*/
+	sram: sram at fff80000 {
+		compatible = "hisilicon,hi6220-sramctrl", "syscon";
+		reg = <0x0 0xfff80000 0x0 0x12000>;
+	};
+
 -----------------------------------------------------------------------
 Hisilicon HiP01 system controller
 
-- 
1.9.1

[PATCH v3 3/5] dt-bindings: clk: Hi6220: Document stub clock driver

[Leo Yan]

Document the new compatible for stub clock driver which is used for CPU
and DDR's dynamic frequency scaling.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 .../devicetree/bindings/clock/hi6220-clock.txt        | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/clock/hi6220-clock.txt b/Documentation/devicetree/bindings/clock/hi6220-clock.txt
index 259e30a..e4d5fea 100644
--- a/Documentation/devicetree/bindings/clock/hi6220-clock.txt
+++ b/Documentation/devicetree/bindings/clock/hi6220-clock.txt
@@ -15,19 +15,36 @@ Required Properties:
 	- "hisilicon,hi6220-sysctrl"
 	- "hisilicon,hi6220-mediactrl"
 	- "hisilicon,hi6220-pmctrl"
+	- "hisilicon,hi6220-stub-clk"
 
 - reg: physical base address of the controller and length of memory mapped
   region.
 
 - #clock-cells: should be 1.
 
-For example:
+Optional Properties:
+
+- hisilicon,hi6220-clk-sram: phandle to the syscon managing the SoC internal sram;
+  the driver need use the sram to pass parameters for frequency change.
+
+- mboxes: use the label reference for the mailbox as the first parameter, the
+  second parameter is the channel number.
+
+Example 1:
 	sys_ctrl: sys_ctrl at f7030000 {
 		compatible = "hisilicon,hi6220-sysctrl", "syscon";
 		reg = <0x0 0xf7030000 0x0 0x2000>;
 		#clock-cells = <1>;
 	};
 
+Example 2:
+	stub_clock: stub_clock {
+		compatible = "hisilicon,hi6220-stub-clk";
+		hisilicon,hi6220-clk-sram = <&sram>;
+		#clock-cells = <1>;
+		mboxes = <&mailbox 1>;
+	};
+
 Each clock is assigned an identifier and client nodes use this identifier
 to specify the clock which they consume.
 
-- 
1.9.1

[PATCH v3 4/5] clk: Hi6220: add stub clock driver

[Leo Yan]

On Hi6220, there have some clocks which can use mailbox channel to send
messages to power controller to change frequency; this includes CPU, GPU
and DDR clocks.

For dynamic frequency scaling, firstly need write the frequency value to
SRAM region, and then send message to mailbox to trigger power controller
to handle this requirement. This driver will use syscon APIs to pass SRAM
memory region and use common mailbox APIs for channels accessing.

This init driver will support cpu frequency change firstly.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 drivers/clk/hisilicon/Kconfig           |   2 +-
 drivers/clk/hisilicon/Makefile          |   2 +-
 drivers/clk/hisilicon/clk-hi6220-stub.c | 279 ++++++++++++++++++++++++++++++++
 3 files changed, 281 insertions(+), 2 deletions(-)
 create mode 100644 drivers/clk/hisilicon/clk-hi6220-stub.c

diff --git a/drivers/clk/hisilicon/Kconfig b/drivers/clk/hisilicon/Kconfig
index b4165ba..2c16807 100644
--- a/drivers/clk/hisilicon/Kconfig
+++ b/drivers/clk/hisilicon/Kconfig
@@ -1,6 +1,6 @@
 config COMMON_CLK_HI6220
 	bool "Hi6220 Clock Driver"
-	depends on ARCH_HISI || COMPILE_TEST
+	depends on (ARCH_HISI || COMPILE_TEST) && MAILBOX
 	default ARCH_HISI
 	help
 	  Build the Hisilicon Hi6220 clock driver based on the common clock framework.
diff --git a/drivers/clk/hisilicon/Makefile b/drivers/clk/hisilicon/Makefile
index 48f0116..4a1001a 100644
--- a/drivers/clk/hisilicon/Makefile
+++ b/drivers/clk/hisilicon/Makefile
@@ -7,4 +7,4 @@ obj-y	+= clk.o clkgate-separated.o clkdivider-hi6220.o
 obj-$(CONFIG_ARCH_HI3xxx)	+= clk-hi3620.o
 obj-$(CONFIG_ARCH_HIP04)	+= clk-hip04.o
 obj-$(CONFIG_ARCH_HIX5HD2)	+= clk-hix5hd2.o
-obj-$(CONFIG_COMMON_CLK_HI6220)	+= clk-hi6220.o
+obj-$(CONFIG_COMMON_CLK_HI6220)	+= clk-hi6220.o clk-hi6220-stub.o
diff --git a/drivers/clk/hisilicon/clk-hi6220-stub.c b/drivers/clk/hisilicon/clk-hi6220-stub.c
new file mode 100644
index 0000000..0931666
--- /dev/null
+++ b/drivers/clk/hisilicon/clk-hi6220-stub.c
@@ -0,0 +1,279 @@
+/*
+ * Hi6220 stub clock driver
+ *
+ * Copyright (c) 2015 Hisilicon Limited.
+ * Copyright (c) 2015 Linaro Limited.
+ *
+ * Author: Leo Yan <leo.yan@linaro.org>
+ *
+ * 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/clkdev.h>
+#include <linux/clk-provider.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/mfd/syscon.h>
+#include <linux/mailbox_client.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/kernel.h>
+
+#include <dt-bindings/clock/hi6220-clock.h>
+
+/* Stub clocks id */
+#define HI6220_STUB_ACPU0		0
+#define HI6220_STUB_ACPU1		1
+#define HI6220_STUB_GPU			2
+#define HI6220_STUB_DDR			5
+
+/* Mailbox message */
+#define HI6220_MBOX_MSG_LEN		8
+
+#define HI6220_MBOX_FREQ		(0xA)
+#define HI6220_MBOX_CMD_SET		(0x3)
+#define HI6220_MBOX_OBJ_AP		(0x0)
+
+/* CPU dynamic frequency scaling */
+#define ACPU_DFS_FREQ_MAX		(0x1724)
+#define ACPU_DFS_CUR_FREQ		(0x17CC)
+#define ACPU_DFS_FLAG			(0x1B30)
+#define ACPU_DFS_FREQ_REQ		(0x1B34)
+#define ACPU_DFS_FREQ_LMT		(0x1B38)
+#define ACPU_DFS_LOCK_FLAG		(0xAEAEAEAE)
+
+#define to_stub_clk(hw) container_of(hw, struct hi6220_stub_clk, hw)
+
+struct hi6220_stub_clk {
+	u32 id;
+	u32 rate;
+
+	struct device *dev;
+	struct clk_hw hw;
+
+	struct regmap *dfs_map;
+	struct mbox_client cl;
+	struct mbox_chan *mbox;
+};
+
+struct hi6220_mbox_msg {
+	unsigned char type;
+	unsigned char cmd;
+	unsigned char obj;
+	unsigned char src;
+	unsigned char para[4];
+};
+
+union hi6220_mbox_data {
+	unsigned int data[HI6220_MBOX_MSG_LEN];
+	struct hi6220_mbox_msg msg;
+};
+
+static unsigned int hi6220_acpu_get_freq(struct hi6220_stub_clk *stub_clk)
+{
+	unsigned int freq;
+
+	regmap_read(stub_clk->dfs_map, ACPU_DFS_CUR_FREQ, &freq);
+	return freq;
+}
+
+static int hi6220_acpu_set_freq(struct hi6220_stub_clk *stub_clk,
+				unsigned int freq)
+{
+	union hi6220_mbox_data data;
+
+	stub_clk->mbox = mbox_request_channel(&stub_clk->cl, 0);
+	if (IS_ERR(stub_clk->mbox)) {
+		dev_err(stub_clk->dev, "failed get mailbox channel\n");
+		return PTR_ERR(stub_clk->mbox);
+	};
+
+	/* set the frequency in sram */
+	regmap_write(stub_clk->dfs_map, ACPU_DFS_FREQ_REQ, freq);
+
+	/* compound mailbox message */
+	data.msg.type = HI6220_MBOX_FREQ;
+	data.msg.cmd  = HI6220_MBOX_CMD_SET;
+	data.msg.obj  = HI6220_MBOX_OBJ_AP;
+	data.msg.src  = HI6220_MBOX_OBJ_AP;
+
+	mbox_send_message(stub_clk->mbox, &data);
+	mbox_free_channel(stub_clk->mbox);
+	return 0;
+}
+
+static int hi6220_acpu_round_freq(struct hi6220_stub_clk *stub_clk,
+				  unsigned int freq)
+{
+	unsigned int limit_flag, limit_freq = UINT_MAX;
+	unsigned int max_freq;
+
+	/* check the constrainted frequency */
+	regmap_read(stub_clk->dfs_map, ACPU_DFS_FLAG, &limit_flag);
+	if (limit_flag == ACPU_DFS_LOCK_FLAG)
+		regmap_read(stub_clk->dfs_map, ACPU_DFS_FREQ_LMT, &limit_freq);
+
+	/* check the supported maximum frequency */
+	regmap_read(stub_clk->dfs_map, ACPU_DFS_FREQ_MAX, &max_freq);
+
+	/* calculate the real maximum frequency */
+	max_freq = min(max_freq, limit_freq);
+
+	if (WARN_ON(freq > max_freq))
+		freq = max_freq;
+
+	return freq;
+}
+
+static unsigned long hi6220_stub_clk_recalc_rate(struct clk_hw *hw,
+		unsigned long parent_rate)
+{
+	u32 rate = 0;
+	struct hi6220_stub_clk *stub_clk = to_stub_clk(hw);
+
+	switch (stub_clk->id) {
+	case HI6220_STUB_ACPU0:
+		rate = hi6220_acpu_get_freq(stub_clk);
+
+		/* convert from KHz to Hz */
+		rate *= 1000;
+		break;
+
+	default:
+		dev_err(stub_clk->dev, "%s: un-supported clock id %d\n",
+			__func__, stub_clk->id);
+		break;
+	}
+
+	return rate;
+}
+
+static int hi6220_stub_clk_set_rate(struct clk_hw *hw, unsigned long rate,
+                    unsigned long parent_rate)
+{
+	struct hi6220_stub_clk *stub_clk = to_stub_clk(hw);
+	unsigned long new_rate = rate / 1000;  /* Khz */
+	int ret = 0;
+
+	switch (stub_clk->id) {
+	case HI6220_STUB_ACPU0:
+		ret = hi6220_acpu_set_freq(stub_clk, new_rate);
+		if (ret < 0)
+			return ret;
+
+		break;
+
+	default:
+		dev_err(stub_clk->dev, "%s: un-supported clock id %d\n",
+			__func__, stub_clk->id);
+		break;
+	}
+
+	stub_clk->rate = new_rate;
+
+	pr_debug("%s: set rate=%ldKhz\n", __func__, new_rate);
+	return ret;
+}
+
+static long hi6220_stub_clk_round_rate(struct clk_hw *hw, unsigned long rate,
+		unsigned long *parent_rate)
+{
+	struct hi6220_stub_clk *stub_clk = to_stub_clk(hw);
+	unsigned long new_rate = rate / 1000;  /* Khz */
+
+	switch (stub_clk->id) {
+	case HI6220_STUB_ACPU0:
+		new_rate = hi6220_acpu_round_freq(stub_clk, new_rate);
+
+		/* convert from KHz to Hz */
+		new_rate *= 1000;
+		break;
+
+	default:
+		dev_err(stub_clk->dev, "%s: un-supported clock id %d\n",
+			__func__, stub_clk->id);
+		break;
+	}
+
+	return new_rate;
+}
+
+static struct clk_ops hi6220_stub_clk_ops = {
+	.recalc_rate	= hi6220_stub_clk_recalc_rate,
+	.round_rate	= hi6220_stub_clk_round_rate,
+	.set_rate	= hi6220_stub_clk_set_rate,
+};
+
+static int hi6220_stub_clk_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct clk_init_data init;
+	struct hi6220_stub_clk *stub_clk;
+	struct clk *clk;
+	struct device_node *np = pdev->dev.of_node;
+
+	stub_clk = devm_kzalloc(&pdev->dev, sizeof(*stub_clk), GFP_KERNEL);
+	if (!stub_clk)
+		return -ENOMEM;
+
+	stub_clk->dfs_map = syscon_regmap_lookup_by_phandle(np,
+				"hisilicon,hi6220-clk-sram");
+	if (IS_ERR(stub_clk->dfs_map)) {
+		dev_err(dev, "failed to get sram regmap\n");
+		return PTR_ERR(stub_clk->dfs_map);
+	}
+
+	stub_clk->hw.init = &init;
+	stub_clk->dev = dev;
+	stub_clk->id = HI6220_STUB_ACPU0;
+
+	/* Use mailbox client with blocking mode */
+	stub_clk->cl.dev = &pdev->dev;
+	stub_clk->cl.tx_done = NULL;
+	stub_clk->cl.tx_block = true;
+	stub_clk->cl.tx_tout = 500;
+	stub_clk->cl.knows_txdone = false;
+
+	init.name = "acpu0";
+	init.ops = &hi6220_stub_clk_ops;
+	init.num_parents = 0;
+	init.flags = CLK_IS_ROOT;
+
+	clk = clk_register(NULL, &stub_clk->hw);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	of_clk_add_provider(pdev->dev.of_node, of_clk_src_simple_get, clk);
+
+	/* initialize buffer to zero */
+	regmap_write(stub_clk->dfs_map, ACPU_DFS_FLAG, 0x0);
+	regmap_write(stub_clk->dfs_map, ACPU_DFS_FREQ_REQ, 0x0);
+	regmap_write(stub_clk->dfs_map, ACPU_DFS_FREQ_LMT, 0x0);
+
+	dev_dbg(&pdev->dev, "Registered clock '%s'\n", init.name);
+	return 0;
+}
+
+static const struct of_device_id hi6220_stub_clk_of_match[] = {
+	{ .compatible = "hisilicon,hi6220-stub-clk", },
+	{}
+};
+
+static struct platform_driver hi6220_stub_clk_driver = {
+	.driver	= {
+		.name = "hi6220-stub-clk",
+		.of_match_table = of_match_ptr(hi6220_stub_clk_of_match),
+	},
+	.probe = hi6220_stub_clk_probe,
+};
+
+static int __init hi6220_stub_clk_init(void)
+{
+	return platform_driver_register(&hi6220_stub_clk_driver);
+}
+core_initcall(hi6220_stub_clk_init);
-- 
1.9.1

[PATCH v3 5/5] arm64: dts: add Hi6220's stub clock node

[Leo Yan]

Enable SRAM node and stub clock node for Hi6220; furthermore
add the CPU's clock so it will be used by cpufreq-dt driver.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 arch/arm64/boot/dts/hisilicon/hi6220.dtsi | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
index b42a9b7..94b8310 100644
--- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
@@ -57,6 +57,17 @@
 			device_type = "cpu";
 			reg = <0x0 0x0>;
 			enable-method = "psci";
+			clocks = <&stub_clock 0>;
+			clock-latency = <0>;
+			operating-points = <
+				/* kHz */
+				1200000  0
+				960000   0
+				729000   0
+				432000   0
+				208000   0
+			>;
+			#cooling-cells = <2>; /* min followed by max */
 		};
 
 		cpu1: cpu at 1 {
@@ -136,6 +147,11 @@
 		#size-cells = <2>;
 		ranges;
 
+		sram: sram at fff80000 {
+			compatible = "hisilicon,hi6220-sramctrl", "syscon";
+			reg = <0x0 0xfff80000 0x0 0x12000>;
+		};
+
 		ao_ctrl: ao_ctrl at f7800000 {
 			compatible = "hisilicon,hi6220-aoctrl", "syscon";
 			reg = <0x0 0xf7800000 0x0 0x2000>;
@@ -160,6 +176,13 @@
 			#clock-cells = <1>;
 		};
 
+		stub_clock: stub_clock {
+			compatible = "hisilicon,hi6220-stub-clk";
+			hisilicon,hi6220-clk-sram = <&sram>;
+			#clock-cells = <1>;
+			mboxes = <&mailbox 1>;
+		};
+
 		uart0: uart at f8015000 {	/* console */
 			compatible = "arm,pl011", "arm,primecell";
 			reg = <0x0 0xf8015000 0x0 0x1000>;
-- 
1.9.1

[PATCH v3 4/5] clk: Hi6220: add stub clock driver

[Stephen Boyd]

On 08/03, Leo Yan wrote:
> diff --git a/drivers/clk/hisilicon/clk-hi6220-stub.c b/drivers/clk/hisilicon/clk-hi6220-stub.c
> new file mode 100644
> index 0000000..0931666
> --- /dev/null
> +++ b/drivers/clk/hisilicon/clk-hi6220-stub.c
> @@ -0,0 +1,279 @@
> +/*
> + * Hi6220 stub clock driver
> + *
> + * Copyright (c) 2015 Hisilicon Limited.
> + * Copyright (c) 2015 Linaro Limited.
> + *
> + * Author: Leo Yan <leo.yan@linaro.org>
> + *
> + * 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/clkdev.h>

Is this include used?

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

Is this include used?

> +#include <linux/err.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/kernel.h>
> +
> +#include <dt-bindings/clock/hi6220-clock.h>
> +
> +/* Stub clocks id */
> +#define HI6220_STUB_ACPU0		0
> +#define HI6220_STUB_ACPU1		1
> +#define HI6220_STUB_GPU			2
> +#define HI6220_STUB_DDR			5
> +
> +/* Mailbox message */
> +#define HI6220_MBOX_MSG_LEN		8
> +
> +#define HI6220_MBOX_FREQ		(0xA)
> +#define HI6220_MBOX_CMD_SET		(0x3)
> +#define HI6220_MBOX_OBJ_AP		(0x0)
> +
> +/* CPU dynamic frequency scaling */
> +#define ACPU_DFS_FREQ_MAX		(0x1724)
> +#define ACPU_DFS_CUR_FREQ		(0x17CC)
> +#define ACPU_DFS_FLAG			(0x1B30)
> +#define ACPU_DFS_FREQ_REQ		(0x1B34)
> +#define ACPU_DFS_FREQ_LMT		(0x1B38)
> +#define ACPU_DFS_LOCK_FLAG		(0xAEAEAEAE)

We don't need parenthesis around single values in these macros.

> +
> +#define to_stub_clk(hw) container_of(hw, struct hi6220_stub_clk, hw)
> +
> +struct hi6220_stub_clk {
> +	u32 id;
> +	u32 rate;
> +
> +	struct device *dev;
> +	struct clk_hw hw;
> +
> +	struct regmap *dfs_map;
> +	struct mbox_client cl;
> +	struct mbox_chan *mbox;
> +};
> +
> +struct hi6220_mbox_msg {
> +	unsigned char type;
> +	unsigned char cmd;
> +	unsigned char obj;
> +	unsigned char src;
> +	unsigned char para[4];
> +};
> +
> +union hi6220_mbox_data {
> +	unsigned int data[HI6220_MBOX_MSG_LEN];
> +	struct hi6220_mbox_msg msg;
> +};
> +
> +static unsigned int hi6220_acpu_get_freq(struct hi6220_stub_clk *stub_clk)
> +{
> +	unsigned int freq;
> +
> +	regmap_read(stub_clk->dfs_map, ACPU_DFS_CUR_FREQ, &freq);
> +	return freq;
> +}
> +
> +static int hi6220_acpu_set_freq(struct hi6220_stub_clk *stub_clk,
> +				unsigned int freq)
> +{
> +	union hi6220_mbox_data data;
> +
> +	stub_clk->mbox = mbox_request_channel(&stub_clk->cl, 0);

Why not request the channel once in probe?

> +	if (IS_ERR(stub_clk->mbox)) {
> +		dev_err(stub_clk->dev, "failed get mailbox channel\n");
> +		return PTR_ERR(stub_clk->mbox);
> +	};
> +
> +	/* set the frequency in sram */
> +	regmap_write(stub_clk->dfs_map, ACPU_DFS_FREQ_REQ, freq);
> +
> +	/* compound mailbox message */
> +	data.msg.type = HI6220_MBOX_FREQ;
> +	data.msg.cmd  = HI6220_MBOX_CMD_SET;
> +	data.msg.obj  = HI6220_MBOX_OBJ_AP;
> +	data.msg.src  = HI6220_MBOX_OBJ_AP;
> +
> +	mbox_send_message(stub_clk->mbox, &data);
> +	mbox_free_channel(stub_clk->mbox);
> +	return 0;
> +}
> +
> +static int hi6220_acpu_round_freq(struct hi6220_stub_clk *stub_clk,
> +				  unsigned int freq)
> +{
> +	unsigned int limit_flag, limit_freq = UINT_MAX;
> +	unsigned int max_freq;
> +
> +	/* check the constrainted frequency */

s/constrainted/constrained/ ?

> +	regmap_read(stub_clk->dfs_map, ACPU_DFS_FLAG, &limit_flag);
> +	if (limit_flag == ACPU_DFS_LOCK_FLAG)
> +		regmap_read(stub_clk->dfs_map, ACPU_DFS_FREQ_LMT, &limit_freq);
> +
> +	/* check the supported maximum frequency */
> +	regmap_read(stub_clk->dfs_map, ACPU_DFS_FREQ_MAX, &max_freq);
> +
> +	/* calculate the real maximum frequency */
> +	max_freq = min(max_freq, limit_freq);
> +
> +	if (WARN_ON(freq > max_freq))
> +		freq = max_freq;
> +
> +	return freq;
> +}
> +
> +static unsigned long hi6220_stub_clk_recalc_rate(struct clk_hw *hw,
> +		unsigned long parent_rate)
> +{
> +	u32 rate = 0;
> +	struct hi6220_stub_clk *stub_clk = to_stub_clk(hw);
> +
> +	switch (stub_clk->id) {
> +	case HI6220_STUB_ACPU0:
> +		rate = hi6220_acpu_get_freq(stub_clk);
> +
> +		/* convert from KHz to Hz */

s/KHz/kHz/ ?

> +		rate *= 1000;
> +		break;
> +
> +	default:
> +		dev_err(stub_clk->dev, "%s: un-supported clock id %d\n",
> +			__func__, stub_clk->id);
> +		break;
> +	}
> +
> +	return rate;
> +}
> +
> +static int hi6220_stub_clk_set_rate(struct clk_hw *hw, unsigned long rate,
> +                    unsigned long parent_rate)
> +{
> +	struct hi6220_stub_clk *stub_clk = to_stub_clk(hw);
> +	unsigned long new_rate = rate / 1000;  /* Khz */

s/KHz/kHz/ ?

> +	int ret = 0;
> +
> +	switch (stub_clk->id) {
> +	case HI6220_STUB_ACPU0:
> +		ret = hi6220_acpu_set_freq(stub_clk, new_rate);
> +		if (ret < 0)
> +			return ret;
> +
> +		break;
> +
> +	default:
> +		dev_err(stub_clk->dev, "%s: un-supported clock id %d\n",
> +			__func__, stub_clk->id);
> +		break;
> +	}
> +
> +	stub_clk->rate = new_rate;

Why do we store away the rate? Is this used anywhere?

> +
> +	pr_debug("%s: set rate=%ldKhz\n", __func__, new_rate);
> +	return ret;
> +}
> +
> +static long hi6220_stub_clk_round_rate(struct clk_hw *hw, unsigned long rate,
> +		unsigned long *parent_rate)
> +{
> +	struct hi6220_stub_clk *stub_clk = to_stub_clk(hw);
> +	unsigned long new_rate = rate / 1000;  /* Khz */

s/KHz/kHz/ ?

> +
> +	switch (stub_clk->id) {
> +	case HI6220_STUB_ACPU0:
> +		new_rate = hi6220_acpu_round_freq(stub_clk, new_rate);
> +
> +		/* convert from KHz to Hz */

s/KHz/kHz/ ?

> +		new_rate *= 1000;
> +		break;
> +
> +	default:
> +		dev_err(stub_clk->dev, "%s: un-supported clock id %d\n",
> +			__func__, stub_clk->id);
> +		break;
> +	}
> +
> +	return new_rate;
> +}
> +
> +static struct clk_ops hi6220_stub_clk_ops = {

const?

> +	.recalc_rate	= hi6220_stub_clk_recalc_rate,
> +	.round_rate	= hi6220_stub_clk_round_rate,
> +	.set_rate	= hi6220_stub_clk_set_rate,
> +};
> +
> +static int hi6220_stub_clk_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct clk_init_data init;
> +	struct hi6220_stub_clk *stub_clk;
> +	struct clk *clk;
> +	struct device_node *np = pdev->dev.of_node;
> +
> +	stub_clk = devm_kzalloc(&pdev->dev, sizeof(*stub_clk), GFP_KERNEL);

just use dev?

> +	if (!stub_clk)
> +		return -ENOMEM;
> +
> +	stub_clk->dfs_map = syscon_regmap_lookup_by_phandle(np,
> +				"hisilicon,hi6220-clk-sram");
> +	if (IS_ERR(stub_clk->dfs_map)) {
> +		dev_err(dev, "failed to get sram regmap\n");
> +		return PTR_ERR(stub_clk->dfs_map);
> +	}
> +
> +	stub_clk->hw.init = &init;
> +	stub_clk->dev = dev;
> +	stub_clk->id = HI6220_STUB_ACPU0;
> +
> +	/* Use mailbox client with blocking mode */
> +	stub_clk->cl.dev = &pdev->dev;

just use dev?

> +	stub_clk->cl.tx_done = NULL;
> +	stub_clk->cl.tx_block = true;
> +	stub_clk->cl.tx_tout = 500;
> +	stub_clk->cl.knows_txdone = false;
> +
> +	init.name = "acpu0";
> +	init.ops = &hi6220_stub_clk_ops;
> +	init.num_parents = 0;
> +	init.flags = CLK_IS_ROOT;
> +
> +	clk = clk_register(NULL, &stub_clk->hw);

why not devm_clk_register?

> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +
> +	of_clk_add_provider(pdev->dev.of_node, of_clk_src_simple_get, clk);

And check this for an error return value? Also, just use dev?

> +
> +	/* initialize buffer to zero */
> +	regmap_write(stub_clk->dfs_map, ACPU_DFS_FLAG, 0x0);
> +	regmap_write(stub_clk->dfs_map, ACPU_DFS_FREQ_REQ, 0x0);
> +	regmap_write(stub_clk->dfs_map, ACPU_DFS_FREQ_LMT, 0x0);
> +
> +	dev_dbg(&pdev->dev, "Registered clock '%s'\n", init.name);

just use dev?

> +	return 0;
> +}
> +
> +static const struct of_device_id hi6220_stub_clk_of_match[] = {
> +	{ .compatible = "hisilicon,hi6220-stub-clk", },
> +	{}
> +};
> +
> +static struct platform_driver hi6220_stub_clk_driver = {
> +	.driver	= {
> +		.name = "hi6220-stub-clk",
> +		.of_match_table = of_match_ptr(hi6220_stub_clk_of_match),

We can leave off of_match_ptr() here.

> +	},
> +	.probe = hi6220_stub_clk_probe,
> +};

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

[PATCH v3 1/5] clk: hisi: refine parameter checking for init

[Stephen Boyd]

On 08/03, Leo Yan wrote:
> *of_iomap()* will check the device node pointer, and if the pointer is
> NULL it will return error code. So refine clock's init flow by checking
> the device node with this simple way; and polish a little for the print
> out message.
> 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---

Applied to clk-next

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

[PATCH v3 4/5] clk: Hi6220: add stub clock driver

[Leo Yan]

Hi Stephen,

On Mon, Aug 03, 2015 at 02:37:52PM -0700, Stephen Boyd wrote:
> On 08/03, Leo Yan wrote:
> > diff --git a/drivers/clk/hisilicon/clk-hi6220-stub.c b/drivers/clk/hisilicon/clk-hi6220-stub.c
> > new file mode 100644
> > index 0000000..0931666
> > --- /dev/null
> > +++ b/drivers/clk/hisilicon/clk-hi6220-stub.c
> > @@ -0,0 +1,279 @@
> > +/*
> > + * Hi6220 stub clock driver
> > + *
> > + * Copyright (c) 2015 Hisilicon Limited.
> > + * Copyright (c) 2015 Linaro Limited.
> > + *
> > + * Author: Leo Yan <leo.yan@linaro.org>
> > + *
> > + * 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/clkdev.h>
> 
> Is this include used?
> 
> > +#include <linux/clk-provider.h>
> > +#include <linux/clk.h>
> 
> Is this include used?
> 
> > +#include <linux/err.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/mailbox_client.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
> > +#include <linux/kernel.h>
> > +
> > +#include <dt-bindings/clock/hi6220-clock.h>
> > +
> > +/* Stub clocks id */
> > +#define HI6220_STUB_ACPU0		0
> > +#define HI6220_STUB_ACPU1		1
> > +#define HI6220_STUB_GPU			2
> > +#define HI6220_STUB_DDR			5
> > +
> > +/* Mailbox message */
> > +#define HI6220_MBOX_MSG_LEN		8
> > +
> > +#define HI6220_MBOX_FREQ		(0xA)
> > +#define HI6220_MBOX_CMD_SET		(0x3)
> > +#define HI6220_MBOX_OBJ_AP		(0x0)
> > +
> > +/* CPU dynamic frequency scaling */
> > +#define ACPU_DFS_FREQ_MAX		(0x1724)
> > +#define ACPU_DFS_CUR_FREQ		(0x17CC)
> > +#define ACPU_DFS_FLAG			(0x1B30)
> > +#define ACPU_DFS_FREQ_REQ		(0x1B34)
> > +#define ACPU_DFS_FREQ_LMT		(0x1B38)
> > +#define ACPU_DFS_LOCK_FLAG		(0xAEAEAEAE)
> 
> We don't need parenthesis around single values in these macros.
> 
> > +
> > +#define to_stub_clk(hw) container_of(hw, struct hi6220_stub_clk, hw)
> > +
> > +struct hi6220_stub_clk {
> > +	u32 id;
> > +	u32 rate;
> > +
> > +	struct device *dev;
> > +	struct clk_hw hw;
> > +
> > +	struct regmap *dfs_map;
> > +	struct mbox_client cl;
> > +	struct mbox_chan *mbox;
> > +};
> > +
> > +struct hi6220_mbox_msg {
> > +	unsigned char type;
> > +	unsigned char cmd;
> > +	unsigned char obj;
> > +	unsigned char src;
> > +	unsigned char para[4];
> > +};
> > +
> > +union hi6220_mbox_data {
> > +	unsigned int data[HI6220_MBOX_MSG_LEN];
> > +	struct hi6220_mbox_msg msg;
> > +};
> > +
> > +static unsigned int hi6220_acpu_get_freq(struct hi6220_stub_clk *stub_clk)
> > +{
> > +	unsigned int freq;
> > +
> > +	regmap_read(stub_clk->dfs_map, ACPU_DFS_CUR_FREQ, &freq);
> > +	return freq;
> > +}
> > +
> > +static int hi6220_acpu_set_freq(struct hi6220_stub_clk *stub_clk,
> > +				unsigned int freq)
> > +{
> > +	union hi6220_mbox_data data;
> > +
> > +	stub_clk->mbox = mbox_request_channel(&stub_clk->cl, 0);
> 
> Why not request the channel once in probe?
> 
> > +	if (IS_ERR(stub_clk->mbox)) {
> > +		dev_err(stub_clk->dev, "failed get mailbox channel\n");
> > +		return PTR_ERR(stub_clk->mbox);
> > +	};
> > +
> > +	/* set the frequency in sram */
> > +	regmap_write(stub_clk->dfs_map, ACPU_DFS_FREQ_REQ, freq);
> > +
> > +	/* compound mailbox message */
> > +	data.msg.type = HI6220_MBOX_FREQ;
> > +	data.msg.cmd  = HI6220_MBOX_CMD_SET;
> > +	data.msg.obj  = HI6220_MBOX_OBJ_AP;
> > +	data.msg.src  = HI6220_MBOX_OBJ_AP;
> > +
> > +	mbox_send_message(stub_clk->mbox, &data);
> > +	mbox_free_channel(stub_clk->mbox);
> > +	return 0;
> > +}
> > +
> > +static int hi6220_acpu_round_freq(struct hi6220_stub_clk *stub_clk,
> > +				  unsigned int freq)
> > +{
> > +	unsigned int limit_flag, limit_freq = UINT_MAX;
> > +	unsigned int max_freq;
> > +
> > +	/* check the constrainted frequency */
> 
> s/constrainted/constrained/ ?
> 
> > +	regmap_read(stub_clk->dfs_map, ACPU_DFS_FLAG, &limit_flag);
> > +	if (limit_flag == ACPU_DFS_LOCK_FLAG)
> > +		regmap_read(stub_clk->dfs_map, ACPU_DFS_FREQ_LMT, &limit_freq);
> > +
> > +	/* check the supported maximum frequency */
> > +	regmap_read(stub_clk->dfs_map, ACPU_DFS_FREQ_MAX, &max_freq);
> > +
> > +	/* calculate the real maximum frequency */
> > +	max_freq = min(max_freq, limit_freq);
> > +
> > +	if (WARN_ON(freq > max_freq))
> > +		freq = max_freq;
> > +
> > +	return freq;
> > +}
> > +
> > +static unsigned long hi6220_stub_clk_recalc_rate(struct clk_hw *hw,
> > +		unsigned long parent_rate)
> > +{
> > +	u32 rate = 0;
> > +	struct hi6220_stub_clk *stub_clk = to_stub_clk(hw);
> > +
> > +	switch (stub_clk->id) {
> > +	case HI6220_STUB_ACPU0:
> > +		rate = hi6220_acpu_get_freq(stub_clk);
> > +
> > +		/* convert from KHz to Hz */
> 
> s/KHz/kHz/ ?
> 
> > +		rate *= 1000;
> > +		break;
> > +
> > +	default:
> > +		dev_err(stub_clk->dev, "%s: un-supported clock id %d\n",
> > +			__func__, stub_clk->id);
> > +		break;
> > +	}
> > +
> > +	return rate;
> > +}
> > +
> > +static int hi6220_stub_clk_set_rate(struct clk_hw *hw, unsigned long rate,
> > +                    unsigned long parent_rate)
> > +{
> > +	struct hi6220_stub_clk *stub_clk = to_stub_clk(hw);
> > +	unsigned long new_rate = rate / 1000;  /* Khz */
> 
> s/KHz/kHz/ ?
> 
> > +	int ret = 0;
> > +
> > +	switch (stub_clk->id) {
> > +	case HI6220_STUB_ACPU0:
> > +		ret = hi6220_acpu_set_freq(stub_clk, new_rate);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		break;
> > +
> > +	default:
> > +		dev_err(stub_clk->dev, "%s: un-supported clock id %d\n",
> > +			__func__, stub_clk->id);
> > +		break;
> > +	}
> > +
> > +	stub_clk->rate = new_rate;
> 
> Why do we store away the rate? Is this used anywhere?
> 
> > +
> > +	pr_debug("%s: set rate=%ldKhz\n", __func__, new_rate);
> > +	return ret;
> > +}
> > +
> > +static long hi6220_stub_clk_round_rate(struct clk_hw *hw, unsigned long rate,
> > +		unsigned long *parent_rate)
> > +{
> > +	struct hi6220_stub_clk *stub_clk = to_stub_clk(hw);
> > +	unsigned long new_rate = rate / 1000;  /* Khz */
> 
> s/KHz/kHz/ ?
> 
> > +
> > +	switch (stub_clk->id) {
> > +	case HI6220_STUB_ACPU0:
> > +		new_rate = hi6220_acpu_round_freq(stub_clk, new_rate);
> > +
> > +		/* convert from KHz to Hz */
> 
> s/KHz/kHz/ ?
> 
> > +		new_rate *= 1000;
> > +		break;
> > +
> > +	default:
> > +		dev_err(stub_clk->dev, "%s: un-supported clock id %d\n",
> > +			__func__, stub_clk->id);
> > +		break;
> > +	}
> > +
> > +	return new_rate;
> > +}
> > +
> > +static struct clk_ops hi6220_stub_clk_ops = {
> 
> const?
> 
> > +	.recalc_rate	= hi6220_stub_clk_recalc_rate,
> > +	.round_rate	= hi6220_stub_clk_round_rate,
> > +	.set_rate	= hi6220_stub_clk_set_rate,
> > +};
> > +
> > +static int hi6220_stub_clk_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct clk_init_data init;
> > +	struct hi6220_stub_clk *stub_clk;
> > +	struct clk *clk;
> > +	struct device_node *np = pdev->dev.of_node;
> > +
> > +	stub_clk = devm_kzalloc(&pdev->dev, sizeof(*stub_clk), GFP_KERNEL);
> 
> just use dev?
> 
> > +	if (!stub_clk)
> > +		return -ENOMEM;
> > +
> > +	stub_clk->dfs_map = syscon_regmap_lookup_by_phandle(np,
> > +				"hisilicon,hi6220-clk-sram");
> > +	if (IS_ERR(stub_clk->dfs_map)) {
> > +		dev_err(dev, "failed to get sram regmap\n");
> > +		return PTR_ERR(stub_clk->dfs_map);
> > +	}
> > +
> > +	stub_clk->hw.init = &init;
> > +	stub_clk->dev = dev;
> > +	stub_clk->id = HI6220_STUB_ACPU0;
> > +
> > +	/* Use mailbox client with blocking mode */
> > +	stub_clk->cl.dev = &pdev->dev;
> 
> just use dev?
> 
> > +	stub_clk->cl.tx_done = NULL;
> > +	stub_clk->cl.tx_block = true;
> > +	stub_clk->cl.tx_tout = 500;
> > +	stub_clk->cl.knows_txdone = false;
> > +
> > +	init.name = "acpu0";
> > +	init.ops = &hi6220_stub_clk_ops;
> > +	init.num_parents = 0;
> > +	init.flags = CLK_IS_ROOT;
> > +
> > +	clk = clk_register(NULL, &stub_clk->hw);
> 
> why not devm_clk_register?
> 
> > +	if (IS_ERR(clk))
> > +		return PTR_ERR(clk);
> > +
> > +	of_clk_add_provider(pdev->dev.of_node, of_clk_src_simple_get, clk);
> 
> And check this for an error return value? Also, just use dev?
> 
> > +
> > +	/* initialize buffer to zero */
> > +	regmap_write(stub_clk->dfs_map, ACPU_DFS_FLAG, 0x0);
> > +	regmap_write(stub_clk->dfs_map, ACPU_DFS_FREQ_REQ, 0x0);
> > +	regmap_write(stub_clk->dfs_map, ACPU_DFS_FREQ_LMT, 0x0);
> > +
> > +	dev_dbg(&pdev->dev, "Registered clock '%s'\n", init.name);
> 
> just use dev?
> 
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id hi6220_stub_clk_of_match[] = {
> > +	{ .compatible = "hisilicon,hi6220-stub-clk", },
> > +	{}
> > +};
> > +
> > +static struct platform_driver hi6220_stub_clk_driver = {
> > +	.driver	= {
> > +		.name = "hi6220-stub-clk",
> > +		.of_match_table = of_match_ptr(hi6220_stub_clk_of_match),
> 
> We can leave off of_match_ptr() here.
> 
> > +	},
> > +	.probe = hi6220_stub_clk_probe,
> > +};


Thanks for review; Accept all comments and will refine for them :)

Thanks,
Leo Yan

[PATCH v3 4/5] clk: Hi6220: add stub clock driver

[Kevin Hilman]

On Sun, Aug 2, 2015 at 6:13 PM, Leo Yan <leo.yan@linaro.org> wrote:
> On Hi6220, there have some clocks which can use mailbox channel to send
> messages to power controller to change frequency; this includes CPU, GPU
> and DDR clocks.
>
> For dynamic frequency scaling, firstly need write the frequency value to
> SRAM region, and then send message to mailbox to trigger power controller
> to handle this requirement. This driver will use syscon APIs to pass SRAM
> memory region and use common mailbox APIs for channels accessing.
>
> This init driver will support cpu frequency change firstly.
>
> Signed-off-by: Leo Yan <leo.yan@linaro.org>

The kernelci.org build/boot bot detected boot failures in
linux-next[1], and the failure was bisected down to this patch (landed
in linux-next as commit c1628a2c416da947f5afac615d53189250fa49cb.

I verifed that reverting this commit on top of next-20150901 gets the
hikey booting again.

Kevin

[1] http://kernelci.org/boot/hi6220-hikey/

[PATCH v3 4/5] clk: Hi6220: add stub clock driver

[Leo Yan]

Hi Kevin,

On Tue, Sep 01, 2015 at 05:28:03PM -0700, Kevin Hilman wrote:
> On Sun, Aug 2, 2015 at 6:13 PM, Leo Yan <leo.yan@linaro.org> wrote:
> > On Hi6220, there have some clocks which can use mailbox channel to send
> > messages to power controller to change frequency; this includes CPU, GPU
> > and DDR clocks.
> >
> > For dynamic frequency scaling, firstly need write the frequency value to
> > SRAM region, and then send message to mailbox to trigger power controller
> > to handle this requirement. This driver will use syscon APIs to pass SRAM
> > memory region and use common mailbox APIs for channels accessing.
> >
> > This init driver will support cpu frequency change firstly.
> >
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> 
> The kernelci.org build/boot bot detected boot failures in
> linux-next[1], and the failure was bisected down to this patch (landed
> in linux-next as commit c1628a2c416da947f5afac615d53189250fa49cb.
> 
> I verifed that reverting this commit on top of next-20150901 gets the
> hikey booting again.

Thanks for reporting. This issue has been confirmed at my side, it's
caused by the patch have added dependency with MAILBOX, will fix this
issue and send patch.

Thanks,
Leo Yan