Re: [PATCH v3 4/5] clk: Hi6220: add stub clock driver
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 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 > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 4/5] clk: Hi6220: add stub clock driver
On Sun, Aug 2, 2015 at 6:13 PM, Leo Yan 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 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/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 4/5] clk: Hi6220: add stub clock driver
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 000..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
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + */
> > +
> > +#include
>
> Is this include used?
>
> > +#include
> > +#include
>
> Is this include used?
>
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include
> > +
> > +/* Stub clocks id */
> > +#define HI6220_STUB_ACPU0 0
> > +#define HI6220_STUB_ACPU1 1
> > +#define HI6220_STUB_GPU2
> > +#define HI6220_STUB_DDR5
> > +
> > +/* Mailbox message */
> > +#define HI6220_MBOX_MSG_LEN8
> > +
> > +#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:
> > + de
Re: [PATCH v3 4/5] clk: Hi6220: add stub clock driver
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 000..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
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#include
Is this include used?
> +#include
> +#include
Is this include used?
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +/* Stub clocks id */
> +#define HI6220_STUB_ACPU00
> +#define HI6220_STUB_ACPU11
> +#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_
[PATCH v3 4/5] clk: Hi6220: add stub clock driver
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
---
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 000..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
+ *
+ * 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
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+
+/* Stub clocks id */
+#define HI6220_STUB_ACPU0 0
+#define HI6220_STUB_ACPU1 1
+#define HI6220_STUB_GPU2
+#define HI6220_STUB_DDR5
+
+/* Mailbox message */
+#define HI6220_MBOX_MSG_LEN8
+
+#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,
+
