[PATCH v4 0/7] rk3399 support ddr frequency scaling

[PATCH v4 0/7] rk3399 support ddr frequency scaling

[Lin Huang]

rk3399 platform have dfi controller can monitor ddr load,
and dcf controller to handle ddr register so we can get the
right ddr frequency and make ddr controller happy work(which
will implement in bl31). So we do ddr frequency scaling with
following flow:

	     kernel                                bl31

	monitor ddr load
		|
		|
	get_target_rate
		|
		|           pass rate to bl31
	clk_set_rate(ddr) --------------------->run dcf flow
		|                                   |
		|                                   |
	wait dcf interrupt<-------------------trigger dcf interrupt  
		|
		|
	      return

Lin Huang (6):
  clk: rockchip: add new clock-type for the ddrclk
  clk: rockchip: rk3399: add SCLK_DDRCLK ID for ddrc
  clk: rockchip: rk3399: add ddrc clock support
  PM / devfreq: event: support rockchip dfi controller
  PM / devfreq: rockchip: add devfreq driver for rk3399 dmc
  drm/rockchip: Add dmc notifier in vop driver


Heiko Stübner (1):
  clk: rockchip: add clock flag parameter when register pll

Lin Huang (6):
  clk: rockchip: add new clock-type for the ddrclk
  clk: rockchip: rk3399: add SCLK_DDRCLK ID for ddrc
  clk: rockchip: rk3399: add ddrc clock support
  PM / devfreq: event: support rockchip dfi controller
  PM / devfreq: rockchip: add devfreq driver for rk3399 dmc
  drm/rockchip: Add dmc notifier in vop driver

 drivers/clk/rockchip/Makefile               |   1 +
 drivers/clk/rockchip/clk-ddr.c              | 146 +++++++++
 drivers/clk/rockchip/clk-pll.c              |   4 +-
 drivers/clk/rockchip/clk-rk3399.c           |  19 ++
 drivers/clk/rockchip/clk.c                  |  11 +-
 drivers/clk/rockchip/clk.h                  |  29 +-
 drivers/devfreq/Kconfig                     |   1 +
 drivers/devfreq/Makefile                    |   1 +
 drivers/devfreq/event/Kconfig               |   7 +
 drivers/devfreq/event/Makefile              |   1 +
 drivers/devfreq/event/rockchip-dfi.c        | 253 +++++++++++++++
 drivers/devfreq/rockchip/Kconfig            |   8 +
 drivers/devfreq/rockchip/Makefile           |   1 +
 drivers/devfreq/rockchip/rk3399_dmc.c       | 473 ++++++++++++++++++++++++++++
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 124 +++++++-
 include/dt-bindings/clock/rk3399-cru.h      |   1 +
 include/soc/rockchip/rockchip_sip.h         |  27 ++
 17 files changed, 1098 insertions(+), 9 deletions(-)
 create mode 100644 drivers/clk/rockchip/clk-ddr.c
 create mode 100644 drivers/devfreq/event/rockchip-dfi.c
 create mode 100644 drivers/devfreq/rockchip/Kconfig
 create mode 100644 drivers/devfreq/rockchip/Makefile
 create mode 100644 drivers/devfreq/rockchip/rk3399_dmc.c
 create mode 100644 include/soc/rockchip/rockchip_sip.h

-- 
1.9.1

[PATCH v4 1/7] clk: rockchip: add clock flag parameter when register pll

[Lin Huang]

From: Heiko Stübner <heiko@sntech.de>

add clock flag parameter so we can pass specific clock flag
(like CLK_GET_RATE_NOCACHE etc..)to pll driver.

Signed-off-by: Heiko Stübner <heiko@sntech.de>
Signed-off-by: Lin Huang <hl@rock-chips.com>
---
Changes in v4:
- None

Changes in v3:
- None

Changes in v2:
- None

Changes in v1:
- None

 drivers/clk/rockchip/clk-pll.c | 4 ++--
 drivers/clk/rockchip/clk.c     | 2 +-
 drivers/clk/rockchip/clk.h     | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/rockchip/clk-pll.c b/drivers/clk/rockchip/clk-pll.c
index 8ac73bc..d824c36 100644
--- a/drivers/clk/rockchip/clk-pll.c
+++ b/drivers/clk/rockchip/clk-pll.c
@@ -864,7 +864,7 @@ struct clk *rockchip_clk_register_pll(struct rockchip_clk_provider *ctx,
 		u8 num_parents, int con_offset, int grf_lock_offset,
 		int lock_shift, int mode_offset, int mode_shift,
 		struct rockchip_pll_rate_table *rate_table,
-		u8 clk_pll_flags)
+		unsigned long flags, u8 clk_pll_flags)
 {
 	const char *pll_parents[3];
 	struct clk_init_data init;
@@ -919,7 +919,7 @@ struct clk *rockchip_clk_register_pll(struct rockchip_clk_provider *ctx,
 	init.name = pll_name;
 
 	/* keep all plls untouched for now */
-	init.flags = CLK_IGNORE_UNUSED;
+	init.flags = flags | CLK_IGNORE_UNUSED;
 
 	init.parent_names = &parent_names[0];
 	init.num_parents = 1;
diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c
index f0a8be1..9a046f1 100644
--- a/drivers/clk/rockchip/clk.c
+++ b/drivers/clk/rockchip/clk.c
@@ -390,7 +390,7 @@ void __init rockchip_clk_register_plls(struct rockchip_clk_provider *ctx,
 				list->con_offset, grf_lock_offset,
 				list->lock_shift, list->mode_offset,
 				list->mode_shift, list->rate_table,
-				list->pll_flags);
+				list->flags, list->pll_flags);
 		if (IS_ERR(clk)) {
 			pr_err("%s: failed to register clock %s\n", __func__,
 				list->name);
diff --git a/drivers/clk/rockchip/clk.h b/drivers/clk/rockchip/clk.h
index 1abb7d0..bac775d 100644
--- a/drivers/clk/rockchip/clk.h
+++ b/drivers/clk/rockchip/clk.h
@@ -238,7 +238,7 @@ struct clk *rockchip_clk_register_pll(struct rockchip_clk_provider *ctx,
 		u8 num_parents, int con_offset, int grf_lock_offset,
 		int lock_shift, int mode_offset, int mode_shift,
 		struct rockchip_pll_rate_table *rate_table,
-		u8 clk_pll_flags);
+		unsigned long flags, u8 clk_pll_flags);
 
 struct rockchip_cpuclk_clksel {
 	int reg;
-- 
1.9.1

Re: [PATCH v4 1/7] clk: rockchip: add clock flag parameter when register pll

[Heiko Stuebner]

Am Freitag, 29. Juli 2016, 15:56:55 schrieb Lin Huang:
> From: Heiko Stübner <heiko@sntech.de>
> 
> add clock flag parameter so we can pass specific clock flag
> (like CLK_GET_RATE_NOCACHE etc..)to pll driver.
> 
> Signed-off-by: Heiko Stübner <heiko@sntech.de>
> Signed-off-by: Lin Huang <hl@rock-chips.com>

applied to my clock branch for 4.9 after some minor edits on the commit 
message.


Thanks
Heiko

Re: [PATCH v4 1/7] clk: rockchip: add clock flag parameter when register pll

[hl]

Hi Heiko,

On 2016年08月05日 06:37, Heiko Stuebner wrote:
> Am Freitag, 29. Juli 2016, 15:56:55 schrieb Lin Huang:
>> From: Heiko Stübner <heiko@sntech.de>
>>
>> add clock flag parameter so we can pass specific clock flag
>> (like CLK_GET_RATE_NOCACHE etc..)to pll driver.
>>
>> Signed-off-by: Heiko Stübner <heiko@sntech.de>
>> Signed-off-by: Lin Huang <hl@rock-chips.com>
> applied to my clock branch for 4.9 after some minor edits on the commit
> message.
  I can not found your clock branch for 4.9, can you share me the patch 
ID, i want
cherry-pick it to my downstream branch.
>
> Thanks
> Heiko
>
>

Re: [PATCH v4 1/7] clk: rockchip: add clock flag parameter when register pll

[Heiko Stübner]

Hi Lin,

Am Freitag, 5. August 2016, 16:50:49 schrieb hl:
> On 2016年08月05日 06:37, Heiko Stuebner wrote:
> > Am Freitag, 29. Juli 2016, 15:56:55 schrieb Lin Huang:
> >> From: Heiko Stübner <heiko@sntech.de>
> >> 
> >> add clock flag parameter so we can pass specific clock flag
> >> (like CLK_GET_RATE_NOCACHE etc..)to pll driver.
> >> 
> >> Signed-off-by: Heiko Stübner <heiko@sntech.de>
> >> Signed-off-by: Lin Huang <hl@rock-chips.com>
> > 
> > applied to my clock branch for 4.9 after some minor edits on the commit
> > message.
> 
>   I can not found your clock branch for 4.9, can you share me the patch
> ID, i want
> cherry-pick it to my downstream branch.

sorry forgot to push yesterday evening. The patch is at [0].

Please keep in mind, that I'll rebase that branch onto 4.8-rc1 once it 
gets released on sunday. So the commit id is volatile till then.
[Same is true for all my 4.9 branch of course, I just pick patches already
to reduce the number of patches in my inbox :-) ]


Heiko

[0] https://git.kernel.org/cgit/linux/kernel/git/mmind/linux-rockchip.git/commit/?h=v4.9-clk/next&id=24bdd1a5a2cc706b8c4820a1976dd4eed56716e4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v4 2/7] clk: rockchip: add new clock-type for the ddrclk

[Lin Huang]

On new rockchip platform(rk3399 etc), there have dcf controller to
do ddr frequency scaling, and this controller will implement in
arm-trust-firmware. We add a special clock-type to handle that.

Signed-off-by: Lin Huang <hl@rock-chips.com>
---
Changes in v4:
- use arm_smccc_smc() to set/read ddr rate

Changes in v3:
- use sip call to set/read ddr rate

Changes in v2:
- use GENMASK instead val_mask
- use divider_recalc_rate() instead DIV_ROUND_UP_ULL
- cleanup code

Changes in v1:
- None

 drivers/clk/rockchip/Makefile       |   1 +
 drivers/clk/rockchip/clk-ddr.c      | 146 ++++++++++++++++++++++++++++++++++++
 drivers/clk/rockchip/clk.c          |   9 +++
 drivers/clk/rockchip/clk.h          |  27 +++++++
 include/soc/rockchip/rockchip_sip.h |  27 +++++++
 5 files changed, 210 insertions(+)
 create mode 100644 drivers/clk/rockchip/clk-ddr.c
 create mode 100644 include/soc/rockchip/rockchip_sip.h

diff --git a/drivers/clk/rockchip/Makefile b/drivers/clk/rockchip/Makefile
index f47a2fa..b5f2c8e 100644
--- a/drivers/clk/rockchip/Makefile
+++ b/drivers/clk/rockchip/Makefile
@@ -8,6 +8,7 @@ obj-y	+= clk-pll.o
 obj-y	+= clk-cpu.o
 obj-y	+= clk-inverter.o
 obj-y	+= clk-mmc-phase.o
+obj-y	+= clk-ddr.o
 obj-$(CONFIG_RESET_CONTROLLER)	+= softrst.o
 
 obj-y	+= clk-rk3036.o
diff --git a/drivers/clk/rockchip/clk-ddr.c b/drivers/clk/rockchip/clk-ddr.c
new file mode 100644
index 0000000..dd657c6
--- /dev/null
+++ b/drivers/clk/rockchip/clk-ddr.c
@@ -0,0 +1,146 @@
+/*
+ * Copyright (c) 2016 Rockchip Electronics Co. Ltd.
+ * Author: Lin Huang <hl@rock-chips.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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/arm-smccc.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+#include <soc/rockchip/rockchip_sip.h>
+
+#include "clk.h"
+
+struct rockchip_ddrclk {
+	struct clk_hw	hw;
+	void __iomem	*reg_base;
+	int		mux_offset;
+	int		mux_shift;
+	int		mux_width;
+	int		mux_flag;
+	int		div_shift;
+	int		div_width;
+	int		div_flag;
+	spinlock_t	*lock;
+};
+
+#define to_rockchip_ddrclk_hw(hw) container_of(hw, struct rockchip_ddrclk, hw)
+
+static int rockchip_ddrclk_set_rate(struct clk_hw *hw, unsigned long drate,
+				    unsigned long prate)
+{
+	struct rockchip_ddrclk *ddrclk = to_rockchip_ddrclk_hw(hw);
+	unsigned long flags;
+	struct arm_smccc_res res;
+
+	spin_lock_irqsave(ddrclk->lock, flags);
+	arm_smccc_smc(SIP_DDR_FREQ, drate, 0, CONFIG_DRAM_SET_RATE,
+		      0, 0, 0, 0, &res);
+	spin_unlock_irqrestore(ddrclk->lock, flags);
+
+	return res.a0;
+}
+
+static unsigned long
+rockchip_ddrclk_recalc_rate(struct clk_hw *hw,
+			    unsigned long parent_rate)
+{
+	struct arm_smccc_res res;
+
+	arm_smccc_smc(SIP_DDR_FREQ, 0, 0, CONFIG_DRAM_GET_RATE,
+		      0, 0, 0, 0, &res);
+
+	return res.a0;
+}
+
+static long clk_ddrclk_round_rate(struct clk_hw *hw, unsigned long rate,
+				  unsigned long *prate)
+{
+	return rate;
+}
+
+static u8 rockchip_ddrclk_get_parent(struct clk_hw *hw)
+{
+	struct rockchip_ddrclk *ddrclk = to_rockchip_ddrclk_hw(hw);
+	int num_parents = clk_hw_get_num_parents(hw);
+	u32 val;
+
+	val = clk_readl(ddrclk->reg_base +
+			ddrclk->mux_offset) >> ddrclk->mux_shift;
+	val &= GENMASK(ddrclk->mux_width - 1, 0);
+
+	if (val >= num_parents)
+		return -EINVAL;
+
+	return val;
+}
+
+static const struct clk_ops rockchip_ddrclk_ops = {
+	.recalc_rate = rockchip_ddrclk_recalc_rate,
+	.set_rate = rockchip_ddrclk_set_rate,
+	.round_rate = clk_ddrclk_round_rate,
+	.get_parent = rockchip_ddrclk_get_parent,
+};
+
+struct clk *rockchip_clk_register_ddrclk(const char *name, int flags,
+					 const char *const *parent_names,
+					 u8 num_parents, int mux_offset,
+					 int mux_shift, int mux_width,
+					 int mux_flag, int div_shift,
+					 int div_width, int div_flag,
+					 void __iomem *reg_base,
+					 spinlock_t *lock)
+{
+	struct rockchip_ddrclk *ddrclk;
+	struct clk_init_data init;
+	struct clk *clk;
+
+	ddrclk = kzalloc(sizeof(*ddrclk), GFP_KERNEL);
+	if (!ddrclk)
+		return ERR_PTR(-ENOMEM);
+
+	init.name = name;
+	init.parent_names = parent_names;
+	init.num_parents = num_parents;
+	init.ops = &rockchip_ddrclk_ops;
+
+	init.flags = flags;
+	init.flags |= CLK_SET_RATE_NO_REPARENT;
+	init.flags |= CLK_GET_RATE_NOCACHE;
+
+	ddrclk->reg_base = reg_base;
+	ddrclk->lock = lock;
+	ddrclk->hw.init = &init;
+	ddrclk->mux_offset = mux_offset;
+	ddrclk->mux_shift = mux_shift;
+	ddrclk->mux_width = mux_width;
+	ddrclk->mux_flag = mux_flag;
+	ddrclk->div_shift = div_shift;
+	ddrclk->div_width = div_width;
+	ddrclk->div_flag = div_flag;
+
+	clk = clk_register(NULL, &ddrclk->hw);
+	if (IS_ERR(clk)) {
+		pr_err("%s: could not register ddrclk %s\n", __func__,	name);
+		goto free_ddrclk;
+	}
+
+	return clk;
+
+free_ddrclk:
+	kfree(ddrclk);
+
+	return NULL;
+}
diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c
index 9a046f1..0369645 100644
--- a/drivers/clk/rockchip/clk.c
+++ b/drivers/clk/rockchip/clk.c
@@ -489,6 +489,15 @@ void __init rockchip_clk_register_branches(
 				list->gate_offset, list->gate_shift,
 				list->gate_flags, flags, &ctx->lock);
 			break;
+		case branch_ddrc:
+			clk = rockchip_clk_register_ddrclk(
+				list->name, list->flags,
+				list->parent_names, list->num_parents,
+				list->muxdiv_offset, list->mux_shift,
+				list->mux_width, list->mux_flags,
+				list->div_shift, list->div_width,
+				list->div_flags, ctx->reg_base, &ctx->lock);
+			break;
 		}
 
 		/* none of the cases above matched */
diff --git a/drivers/clk/rockchip/clk.h b/drivers/clk/rockchip/clk.h
index bac775d..2e3bccf 100644
--- a/drivers/clk/rockchip/clk.h
+++ b/drivers/clk/rockchip/clk.h
@@ -281,6 +281,13 @@ struct clk *rockchip_clk_register_mmc(const char *name,
 				const char *const *parent_names, u8 num_parents,
 				void __iomem *reg, int shift);
 
+struct clk *rockchip_clk_register_ddrclk(const char *name, int flags,
+			const char *const *parent_names, u8 num_parents,
+			int mux_offset, int mux_shift, int mux_width,
+			int mux_flag, int div_shift, int div_width,
+			int div_flag, void __iomem *reg_base,
+			spinlock_t *lock);
+
 #define ROCKCHIP_INVERTER_HIWORD_MASK	BIT(0)
 
 struct clk *rockchip_clk_register_inverter(const char *name,
@@ -299,6 +306,7 @@ enum rockchip_clk_branch_type {
 	branch_mmc,
 	branch_inverter,
 	branch_factor,
+	branch_ddrc,
 };
 
 struct rockchip_clk_branch {
@@ -488,6 +496,25 @@ struct rockchip_clk_branch {
 		.child		= ch,				\
 	}
 
+#define COMPOSITE_DDRC(_id, cname, pnames, f, mo, ms, mw, mf,	\
+			 ds, dw, df)				\
+	{							\
+		.id		= _id,				\
+		.branch_type	= branch_ddrc,			\
+		.name		= cname,			\
+		.parent_names	= pnames,			\
+		.num_parents	= ARRAY_SIZE(pnames),		\
+		.flags		= f,				\
+		.muxdiv_offset	= mo,				\
+		.mux_shift	= ms,				\
+		.mux_width	= mw,				\
+		.mux_flags	= mf,				\
+		.div_shift	= ds,				\
+		.div_width	= dw,				\
+		.div_flags	= df,				\
+		.gate_offset	= -1,				\
+	}
+
 #define MUX(_id, cname, pnames, f, o, s, w, mf)			\
 	{							\
 		.id		= _id,				\
diff --git a/include/soc/rockchip/rockchip_sip.h b/include/soc/rockchip/rockchip_sip.h
new file mode 100644
index 0000000..1fa7389
--- /dev/null
+++ b/include/soc/rockchip/rockchip_sip.h
@@ -0,0 +1,27 @@
+/*
+ * Copyright (c) 2016, Fuzhou Rockchip Electronics Co., Ltd
+ * Author: Lin Huang <hl@rock-chips.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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 __SOC_ROCKCHIP_SIP_H
+#define __SOC_ROCKCHIP_SIP_H
+
+#define SIP_DDR_FREQ		0xC2000008
+#define CONFIG_DRAM_INIT	0x00
+#define CONFIG_DRAM_SET_RATE	0x01
+#define CONFIG_DRAM_ROUND_RATE	0x02
+#define CONFIG_DRAM_SET_AT_SR	0x03
+#define CONFIG_DRAM_GET_BW	0x04
+#define CONFIG_DRAM_GET_RATE	0x05
+#define CONFIG_DRAM_CLR_IRQ	0x06
+#define CONFIG_DRAM_SET_PARAM	0x07
+
+#endif
-- 
1.9.1

Re: [PATCH v4 2/7] clk: rockchip: add new clock-type for the ddrclk

[Heiko Stübner]

Hi Lin,

Am Freitag, 29. Juli 2016, 15:56:56 schrieb Lin Huang:
> On new rockchip platform(rk3399 etc), there have dcf controller to
> do ddr frequency scaling, and this controller will implement in
> arm-trust-firmware. We add a special clock-type to handle that.
> 
> Signed-off-by: Lin Huang <hl@rock-chips.com>

please also include the ARM people from last time in your list.
The arm_smccc_smc calls look correct on first glance, but there is only
one other example in the kernel [0] outside psci that is using it, so I'd
like some confirmation that we're doing the right thing :-)


[0] http://lxr.free-electrons.com/source/arch/arm/mach-artpec/board-artpec6.c#L55


> diff --git a/drivers/clk/rockchip/clk.h b/drivers/clk/rockchip/clk.h
> index bac775d..2e3bccf 100644
> --- a/drivers/clk/rockchip/clk.h
> +++ b/drivers/clk/rockchip/clk.h
> @@ -281,6 +281,13 @@ struct clk *rockchip_clk_register_mmc(const char *name,
> const char *const *parent_names, u8 num_parents,
>  				void __iomem *reg, int shift);
> 
> +struct clk *rockchip_clk_register_ddrclk(const char *name, int flags,
> +			const char *const *parent_names, u8 num_parents,
> +			int mux_offset, int mux_shift, int mux_width,
> +			int mux_flag, int div_shift, int div_width,
> +			int div_flag, void __iomem *reg_base,
> +			spinlock_t *lock);
> +
>  #define ROCKCHIP_INVERTER_HIWORD_MASK	BIT(0)
> 
>  struct clk *rockchip_clk_register_inverter(const char *name,
> @@ -299,6 +306,7 @@ enum rockchip_clk_branch_type {
>  	branch_mmc,
>  	branch_inverter,
>  	branch_factor,
> +	branch_ddrc,
>  };
> 
>  struct rockchip_clk_branch {
> @@ -488,6 +496,25 @@ struct rockchip_clk_branch {
>  		.child		= ch,				\
>  	}
> 
> +#define COMPOSITE_DDRC(_id, cname, pnames, f, mo, ms, mw, mf,	\
> +			 ds, dw, df)				\
> +	{							\
> +		.id		= _id,				\
> +		.branch_type	= branch_ddrc,			\
> +		.name		= cname,			\
> +		.parent_names	= pnames,			\
> +		.num_parents	= ARRAY_SIZE(pnames),		\
> +		.flags		= f,				\
> +		.muxdiv_offset	= mo,				\
> +		.mux_shift	= ms,				\
> +		.mux_width	= mw,				\
> +		.mux_flags	= mf,				\
> +		.div_shift	= ds,				\
> +		.div_width	= dw,				\
> +		.div_flags	= df,				\

you don't need (nor use) div and mux flags here, please use one flag
param like the inverter type does and maybe directly add a flag like
	ROCKCHIP_DDRCLK_SIP
for this type.

Background being, that the ddr clock mechanism is essentially the same
on all socs and only the method to change the rate varies
(sip on rk3399, scpi on rk3368, something sram-based on rk3288 and before)
so in the future, this driver should hopefully be able to carry all those
different methods.


> +		.gate_offset	= -1,				\
> +	}
> +
>  #define MUX(_id, cname, pnames, f, o, s, w, mf)			\
>  	{							\
>  		.id		= _id,				\
> diff --git a/include/soc/rockchip/rockchip_sip.h
> b/include/soc/rockchip/rockchip_sip.h new file mode 100644
> index 0000000..1fa7389
> --- /dev/null
> +++ b/include/soc/rockchip/rockchip_sip.h
> @@ -0,0 +1,27 @@
> +/*
> + * Copyright (c) 2016, Fuzhou Rockchip Electronics Co., Ltd
> + * Author: Lin Huang <hl@rock-chips.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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 __SOC_ROCKCHIP_SIP_H
> +#define __SOC_ROCKCHIP_SIP_H
> +
> +#define SIP_DDR_FREQ		0xC2000008
> +#define CONFIG_DRAM_INIT	0x00
> +#define CONFIG_DRAM_SET_RATE	0x01
> +#define CONFIG_DRAM_ROUND_RATE	0x02
> +#define CONFIG_DRAM_SET_AT_SR	0x03
> +#define CONFIG_DRAM_GET_BW	0x04
> +#define CONFIG_DRAM_GET_RATE	0x05
> +#define CONFIG_DRAM_CLR_IRQ	0x06
> +#define CONFIG_DRAM_SET_PARAM	0x07

please use a better prefix, something like

RK3399_SIP_DDR_FREQ
RK3399_DRAM_INIT
etc

That interface is most likely pretty rk3399-specific and later socs may
very well use a different interace, so this should not occupy
generic names.

Heiko

Re: [PATCH v4 2/7] clk: rockchip: add new clock-type for the ddrclk

[Heiko Stuebner]

Am Donnerstag, 4. August 2016, 22:23:05 schrieb Heiko Stübner:
> Hi Lin,
> 
> Am Freitag, 29. Juli 2016, 15:56:56 schrieb Lin Huang:
> > On new rockchip platform(rk3399 etc), there have dcf controller to
> > do ddr frequency scaling, and this controller will implement in
> > arm-trust-firmware. We add a special clock-type to handle that.
> > 
> > Signed-off-by: Lin Huang <hl@rock-chips.com>
> 
> please also include the ARM people from last time in your list.
> The arm_smccc_smc calls look correct on first glance, but there is only
> one other example in the kernel [0] outside psci that is using it, so I'd
> like some confirmation that we're doing the right thing :-)
> 
> 
> [0]
> http://lxr.free-electrons.com/source/arch/arm/mach-artpec/board-artpec6.c
> #L55
> > diff --git a/drivers/clk/rockchip/clk.h b/drivers/clk/rockchip/clk.h
> > index bac775d..2e3bccf 100644
> > --- a/drivers/clk/rockchip/clk.h
> > +++ b/drivers/clk/rockchip/clk.h
> > @@ -281,6 +281,13 @@ struct clk *rockchip_clk_register_mmc(const char
> > *name, const char *const *parent_names, u8 num_parents,
> > 
> >  				void __iomem *reg, int shift);
> > 
> > +struct clk *rockchip_clk_register_ddrclk(const char *name, int flags,
> > +			const char *const *parent_names, u8 num_parents,
> > +			int mux_offset, int mux_shift, int mux_width,
> > +			int mux_flag, int div_shift, int div_width,
> > +			int div_flag, void __iomem *reg_base,
> > +			spinlock_t *lock);
> > +
> > 
> >  #define ROCKCHIP_INVERTER_HIWORD_MASK	BIT(0)
> >  
> >  struct clk *rockchip_clk_register_inverter(const char *name,
> > 
> > @@ -299,6 +306,7 @@ enum rockchip_clk_branch_type {
> > 
> >  	branch_mmc,
> >  	branch_inverter,
> >  	branch_factor,
> > 
> > +	branch_ddrc,
> > 
> >  };
> >  
> >  struct rockchip_clk_branch {
> > 
> > @@ -488,6 +496,25 @@ struct rockchip_clk_branch {
> > 
> >  		.child		= ch,				\
> >  	
> >  	}
> > 
> > +#define COMPOSITE_DDRC(_id, cname, pnames, f, mo, ms, mw, mf,	\
> > +			 ds, dw, df)				\
> > +	{							\
> > +		.id		= _id,				\
> > +		.branch_type	= branch_ddrc,			\
> > +		.name		= cname,			\
> > +		.parent_names	= pnames,			\
> > +		.num_parents	= ARRAY_SIZE(pnames),		\
> > +		.flags		= f,				\
> > +		.muxdiv_offset	= mo,				\
> > +		.mux_shift	= ms,				\
> > +		.mux_width	= mw,				\
> > +		.mux_flags	= mf,				\
> > +		.div_shift	= ds,				\
> > +		.div_width	= dw,				\
> > +		.div_flags	= df,				\
> 
> you don't need (nor use) div and mux flags here, please use one flag
> param like the inverter type does and maybe directly add a flag like
> 	ROCKCHIP_DDRCLK_SIP
> for this type.
> 
> Background being, that the ddr clock mechanism is essentially the same
> on all socs and only the method to change the rate varies
> (sip on rk3399, scpi on rk3368, something sram-based on rk3288 and before)
> so in the future, this driver should hopefully be able to carry all those
> different methods.

and maybe name it COMPOSITE_DDRCLK(...)

[PATCH v4 3/7] clk: rockchip: rk3399: add SCLK_DDRCLK ID for ddrc

[Lin Huang]

Signed-off-by: Lin Huang <hl@rock-chips.com>
---
Changes in v4:
-None

Changes in v3:
-None

Changes in v2:
- None 
Changes in v1:
- None

 include/dt-bindings/clock/rk3399-cru.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/dt-bindings/clock/rk3399-cru.h b/include/dt-bindings/clock/rk3399-cru.h
index 50a44cf..8a0f0442 100644
--- a/include/dt-bindings/clock/rk3399-cru.h
+++ b/include/dt-bindings/clock/rk3399-cru.h
@@ -131,6 +131,7 @@
 #define SCLK_DPHY_RX0_CFG		165
 #define SCLK_RMII_SRC			166
 #define SCLK_PCIEPHY_REF100M		167
+#define SCLK_DDRCLK			168
 
 #define DCLK_VOP0			180
 #define DCLK_VOP1			181
-- 
1.9.1

Re: [PATCH v4 3/7] clk: rockchip: rk3399: add SCLK_DDRCLK ID for ddrc

[Heiko Stuebner]

Am Freitag, 29. Juli 2016, 15:56:57 schrieb Lin Huang:
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> ---
> Changes in v4:
> -None
> 
> Changes in v3:
> -None
> 
> Changes in v2:
> - None
> Changes in v1:
> - None
> 
>  include/dt-bindings/clock/rk3399-cru.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/dt-bindings/clock/rk3399-cru.h
> b/include/dt-bindings/clock/rk3399-cru.h index 50a44cf..8a0f0442 100644
> --- a/include/dt-bindings/clock/rk3399-cru.h
> +++ b/include/dt-bindings/clock/rk3399-cru.h
> @@ -131,6 +131,7 @@
>  #define SCLK_DPHY_RX0_CFG		165
>  #define SCLK_RMII_SRC			166
>  #define SCLK_PCIEPHY_REF100M		167
> +#define SCLK_DDRCLK			168

name it SCLK_DDRC instead, somewhat matching the naming in the TRM?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v4 4/7] clk: rockchip: rk3399: add ddrc clock support

[Lin Huang]

add ddrc clock setting, so we can do ddr frequency
scaling on rk3399 platform in future.

Signed-off-by: Lin Huang <hl@rock-chips.com>
---
Changes in v4:
- None

Changes in v3:
- None

Changes in v2:
- remove clk_ddrc_dpll_src from critical clock list

Changes in v1:
- remove ddrc source CLK_IGNORE_UNUSED flag
- move clk_ddrc and clk_ddrc_dpll_src to critical

 drivers/clk/rockchip/clk-rk3399.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c
index d4a1cf0..b7b42d9 100644
--- a/drivers/clk/rockchip/clk-rk3399.c
+++ b/drivers/clk/rockchip/clk-rk3399.c
@@ -118,6 +118,10 @@ PNAME(mux_armclkb_p)				= { "clk_core_b_lpll_src",
 						    "clk_core_b_bpll_src",
 						    "clk_core_b_dpll_src",
 						    "clk_core_b_gpll_src" };
+PNAME(mux_ddrclk_p)				= { "clk_ddrc_lpll_src",
+						    "clk_ddrc_bpll_src",
+						    "clk_ddrc_dpll_src",
+						    "clk_ddrc_gpll_src" };
 PNAME(mux_aclk_cci_p)				= { "cpll_aclk_cci_src",
 						    "gpll_aclk_cci_src",
 						    "npll_aclk_cci_src",
@@ -1377,6 +1381,18 @@ static struct rockchip_clk_branch rk3399_clk_branches[] __initdata = {
 	COMPOSITE_NOMUX(0, "clk_test", "clk_test_pre", CLK_IGNORE_UNUSED,
 			RK3368_CLKSEL_CON(58), 0, 5, DFLAGS,
 			RK3368_CLKGATE_CON(13), 11, GFLAGS),
+
+	/* ddrc */
+	GATE(0, "clk_ddrc_lpll_src", "lpll", 0, RK3399_CLKGATE_CON(3),
+	     0, GFLAGS),
+	GATE(0, "clk_ddrc_bpll_src", "bpll", 0, RK3399_CLKGATE_CON(3),
+	     1, GFLAGS),
+	GATE(0, "clk_ddrc_dpll_src", "dpll", 0, RK3399_CLKGATE_CON(3),
+	     2, GFLAGS),
+	GATE(0, "clk_ddrc_gpll_src", "gpll", 0, RK3399_CLKGATE_CON(3),
+	     3, GFLAGS),
+	COMPOSITE_DDRC(SCLK_DDRCLK, "clk_ddrc", mux_ddrclk_p, 0,
+		       RK3399_CLKSEL_CON(6), 4, 2, MFLAGS, 0, 3, DFLAGS),
 };
 
 static struct rockchip_clk_branch rk3399_clk_pmu_branches[] __initdata = {
@@ -1487,6 +1503,9 @@ static const char *const rk3399_cru_critical_clocks[] __initconst = {
 	"gpll_hclk_perilp1_src",
 	"gpll_aclk_perilp0_src",
 	"gpll_aclk_perihp_src",
+
+	/* ddrc */
+	"clk_ddrc"
 };
 
 static const char *const rk3399_pmucru_critical_clocks[] __initconst = {
-- 
1.9.1

[PATCH v4 5/7] PM / devfreq: event: support rockchip dfi controller

[Lin Huang]

on rk3399 platform, there is dfi conroller can monitor
ddr load, base on this result, we can do ddr freqency
scaling.

Signed-off-by: Lin Huang <hl@rock-chips.com>
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
---
Changes in v4:
- None

Changes in v3:
- None

Changes in v2:
- use clk_disable_unprepare and clk_enable_prepare
- remove clk_enable_prepare in probe
- remove rockchip_dfi_remove function

Changes in v1:
- None

 drivers/devfreq/event/Kconfig        |   7 +
 drivers/devfreq/event/Makefile       |   1 +
 drivers/devfreq/event/rockchip-dfi.c | 253 +++++++++++++++++++++++++++++++++++
 3 files changed, 261 insertions(+)
 create mode 100644 drivers/devfreq/event/rockchip-dfi.c

diff --git a/drivers/devfreq/event/Kconfig b/drivers/devfreq/event/Kconfig
index a11720a..ff9279f 100644
--- a/drivers/devfreq/event/Kconfig
+++ b/drivers/devfreq/event/Kconfig
@@ -22,4 +22,11 @@ config DEVFREQ_EVENT_EXYNOS_PPMU
 	  (Platform Performance Monitoring Unit) counters to estimate the
 	  utilization of each module.
 
+config DEVFREQ_EVENT_ROCKCHIP_DFI
+	tristate "ROCKCHIP DFI DEVFREQ event Driver"
+	depends on ARCH_ROCKCHIP
+	help
+	  This add the devfreq-event driver for Rockchip SoC. It provides DFI
+	  (DDR Monitor Module) driver to count ddr load.
+
 endif # PM_DEVFREQ_EVENT
diff --git a/drivers/devfreq/event/Makefile b/drivers/devfreq/event/Makefile
index be146ea..e3f88fc 100644
--- a/drivers/devfreq/event/Makefile
+++ b/drivers/devfreq/event/Makefile
@@ -1,2 +1,3 @@
 # Exynos DEVFREQ Event Drivers
 obj-$(CONFIG_DEVFREQ_EVENT_EXYNOS_PPMU) += exynos-ppmu.o
+obj-$(CONFIG_DEVFREQ_EVENT_ROCKCHIP_DFI) += rockchip-dfi.o
diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
new file mode 100644
index 0000000..96a0307
--- /dev/null
+++ b/drivers/devfreq/event/rockchip-dfi.c
@@ -0,0 +1,253 @@
+/*
+ * Copyright (c) 2016, Fuzhou Rockchip Electronics Co., Ltd
+ * Author: Lin Huang <hl@rock-chips.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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/devfreq-event.h>
+#include <linux/kernel.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/list.h>
+#include <linux/of.h>
+
+#define RK3399_DMC_NUM_CH	2
+
+/* DDRMON_CTRL */
+#define DDRMON_CTRL	0x04
+#define CLR_DDRMON_CTRL	(0x1f0000 << 0)
+#define LPDDR4_EN	(0x10001 << 4)
+#define HARDWARE_EN	(0x10001 << 3)
+#define LPDDR3_EN	(0x10001 << 2)
+#define SOFTWARE_EN	(0x10001 << 1)
+#define TIME_CNT_EN	(0x10001 << 0)
+
+#define DDRMON_CH0_COUNT_NUM		0x28
+#define DDRMON_CH0_DFI_ACCESS_NUM	0x2c
+#define DDRMON_CH1_COUNT_NUM		0x3c
+#define DDRMON_CH1_DFI_ACCESS_NUM	0x40
+
+/* pmu grf */
+#define PMUGRF_OS_REG2	0x308
+#define DDRTYPE_SHIFT	13
+#define DDRTYPE_MASK	7
+
+enum {
+	DDR3 = 3,
+	LPDDR3 = 6,
+	LPDDR4 = 7,
+	UNUSED = 0xFF
+};
+
+struct dmc_usage {
+	u32 access;
+	u32 total;
+};
+
+struct rockchip_dfi {
+	struct devfreq_event_dev *edev;
+	struct devfreq_event_desc *desc;
+	struct dmc_usage ch_usage[RK3399_DMC_NUM_CH];
+	struct device *dev;
+	void __iomem *regs;
+	struct regmap *regmap_pmu;
+	struct clk *clk;
+};
+
+static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
+{
+	struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
+	void __iomem *dfi_regs = info->regs;
+	u32 val;
+	u32 ddr_type;
+
+	/* get ddr type */
+	regmap_read(info->regmap_pmu, PMUGRF_OS_REG2, &val);
+	ddr_type = (val >> DDRTYPE_SHIFT) & DDRTYPE_MASK;
+
+	/* clear DDRMON_CTRL setting */
+	writel_relaxed(CLR_DDRMON_CTRL, dfi_regs + DDRMON_CTRL);
+
+	/* set ddr type to dfi */
+	if (ddr_type == LPDDR3)
+		writel_relaxed(LPDDR3_EN, dfi_regs + DDRMON_CTRL);
+	else if (ddr_type == LPDDR4)
+		writel_relaxed(LPDDR4_EN, dfi_regs + DDRMON_CTRL);
+
+	/* enable count, use software mode */
+	writel_relaxed(SOFTWARE_EN, dfi_regs + DDRMON_CTRL);
+}
+
+static void rockchip_dfi_stop_hardware_counter(struct devfreq_event_dev *edev)
+{
+	struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
+	void __iomem *dfi_regs = info->regs;
+	u32 val;
+
+	val = readl_relaxed(dfi_regs + DDRMON_CTRL);
+	val &= ~SOFTWARE_EN;
+	writel_relaxed(val, dfi_regs + DDRMON_CTRL);
+}
+
+static int rockchip_dfi_get_busier_ch(struct devfreq_event_dev *edev)
+{
+	struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
+	u32 tmp, max = 0;
+	u32 i, busier_ch = 0;
+	void __iomem *dfi_regs = info->regs;
+
+	rockchip_dfi_stop_hardware_counter(edev);
+
+	/* Find out which channel is busier */
+	for (i = 0; i < RK3399_DMC_NUM_CH; i++) {
+		info->ch_usage[i].access = readl_relaxed(dfi_regs +
+				DDRMON_CH0_DFI_ACCESS_NUM + i * 20);
+		info->ch_usage[i].total = readl_relaxed(dfi_regs +
+				DDRMON_CH0_COUNT_NUM + i * 20);
+		tmp = info->ch_usage[i].access;
+		if (tmp > max) {
+			busier_ch = i;
+			max = tmp;
+		}
+	}
+	rockchip_dfi_start_hardware_counter(edev);
+
+	return busier_ch;
+}
+
+static int rockchip_dfi_disable(struct devfreq_event_dev *edev)
+{
+	struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
+
+	rockchip_dfi_stop_hardware_counter(edev);
+	clk_disable_unprepare(info->clk);
+
+	return 0;
+}
+
+static int rockchip_dfi_enable(struct devfreq_event_dev *edev)
+{
+	struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
+	int ret;
+
+	ret = clk_prepare_enable(info->clk);
+	if (ret) {
+		dev_err(&edev->dev, "failed to enable dfi clk: %d\n", ret);
+		return ret;
+	}
+
+	rockchip_dfi_start_hardware_counter(edev);
+	return 0;
+}
+
+static int rockchip_dfi_set_event(struct devfreq_event_dev *edev)
+{
+	return 0;
+}
+
+static int rockchip_dfi_get_event(struct devfreq_event_dev *edev,
+				  struct devfreq_event_data *edata)
+{
+	struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
+	int busier_ch;
+
+	busier_ch = rockchip_dfi_get_busier_ch(edev);
+
+	edata->load_count = info->ch_usage[busier_ch].access;
+	edata->total_count = info->ch_usage[busier_ch].total;
+
+	return 0;
+}
+
+static const struct devfreq_event_ops rockchip_dfi_ops = {
+	.disable = rockchip_dfi_disable,
+	.enable = rockchip_dfi_enable,
+	.get_event = rockchip_dfi_get_event,
+	.set_event = rockchip_dfi_set_event,
+};
+
+static const struct of_device_id rockchip_dfi_id_match[] = {
+	{ .compatible = "rockchip,rk3399-dfi" },
+	{ },
+};
+
+static int rockchip_dfi_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rockchip_dfi *data;
+	struct resource *res;
+	struct devfreq_event_desc *desc;
+	struct device_node *np = pdev->dev.of_node, *node;
+
+	data = devm_kzalloc(dev, sizeof(struct rockchip_dfi), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	data->regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(data->regs))
+		return PTR_ERR(data->regs);
+
+	data->clk = devm_clk_get(dev, "pclk_ddr_mon");
+	if (IS_ERR(data->clk)) {
+		dev_err(dev, "Cannot get the clk dmc_clk\n");
+		return PTR_ERR(data->clk);
+	};
+
+	/* try to find the optional reference to the pmu syscon */
+	node = of_parse_phandle(np, "rockchip,pmu", 0);
+	if (node) {
+		data->regmap_pmu = syscon_node_to_regmap(node);
+		if (IS_ERR(data->regmap_pmu))
+			return PTR_ERR(data->regmap_pmu);
+	}
+	data->dev = dev;
+
+	desc = devm_kzalloc(dev, sizeof(*desc), GFP_KERNEL);
+	if (!desc)
+		return -ENOMEM;
+
+	desc->ops = &rockchip_dfi_ops;
+	desc->driver_data = data;
+	desc->name = np->name;
+	data->desc = desc;
+
+	data->edev = devm_devfreq_event_add_edev(&pdev->dev, desc);
+	if (IS_ERR(data->edev)) {
+		dev_err(&pdev->dev,
+			"failed to add devfreq-event device\n");
+		return PTR_ERR(data->edev);
+	}
+
+	platform_set_drvdata(pdev, data);
+
+	return 0;
+}
+
+static struct platform_driver rockchip_dfi_driver = {
+	.probe	= rockchip_dfi_probe,
+	.driver = {
+		.name	= "rockchip-dfi",
+		.of_match_table = rockchip_dfi_id_match,
+	},
+};
+module_platform_driver(rockchip_dfi_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Lin Huang <hl@rock-chips.com>");
+MODULE_DESCRIPTION("Rockchip dfi driver");
-- 
1.9.1

Re: [PATCH v4 5/7] PM / devfreq: event: support rockchip dfi controller

[Chanwoo Choi]

Hi Lin,

Because you remove the 'RFC' prefix on patch title,
I think that you better to make the documentation as following: 
- Documentation/devicetree/bindings/devfreq/event/rockchip-dfi.txt

Regards,
Chanwoo Choi

On 2016년 07월 29일 16:56, Lin Huang wrote:
> on rk3399 platform, there is dfi conroller can monitor
> ddr load, base on this result, we can do ddr freqency
> scaling.
> 
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
> Changes in v4:
> - None
> 
> Changes in v3:
> - None
> 
> Changes in v2:
> - use clk_disable_unprepare and clk_enable_prepare
> - remove clk_enable_prepare in probe
> - remove rockchip_dfi_remove function
> 
> Changes in v1:
> - None
> 
>  drivers/devfreq/event/Kconfig        |   7 +
>  drivers/devfreq/event/Makefile       |   1 +
>  drivers/devfreq/event/rockchip-dfi.c | 253 +++++++++++++++++++++++++++++++++++
>  3 files changed, 261 insertions(+)
>  create mode 100644 drivers/devfreq/event/rockchip-dfi.c
> 
> diff --git a/drivers/devfreq/event/Kconfig b/drivers/devfreq/event/Kconfig
> index a11720a..ff9279f 100644
> --- a/drivers/devfreq/event/Kconfig
> +++ b/drivers/devfreq/event/Kconfig
> @@ -22,4 +22,11 @@ config DEVFREQ_EVENT_EXYNOS_PPMU
>  	  (Platform Performance Monitoring Unit) counters to estimate the
>  	  utilization of each module.
>  
> +config DEVFREQ_EVENT_ROCKCHIP_DFI
> +	tristate "ROCKCHIP DFI DEVFREQ event Driver"
> +	depends on ARCH_ROCKCHIP
> +	help
> +	  This add the devfreq-event driver for Rockchip SoC. It provides DFI
> +	  (DDR Monitor Module) driver to count ddr load.
> +
>  endif # PM_DEVFREQ_EVENT
> diff --git a/drivers/devfreq/event/Makefile b/drivers/devfreq/event/Makefile
> index be146ea..e3f88fc 100644
> --- a/drivers/devfreq/event/Makefile
> +++ b/drivers/devfreq/event/Makefile
> @@ -1,2 +1,3 @@
>  # Exynos DEVFREQ Event Drivers
>  obj-$(CONFIG_DEVFREQ_EVENT_EXYNOS_PPMU) += exynos-ppmu.o
> +obj-$(CONFIG_DEVFREQ_EVENT_ROCKCHIP_DFI) += rockchip-dfi.o
> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> new file mode 100644
> index 0000000..96a0307
> --- /dev/null
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -0,0 +1,253 @@
> +/*
> + * Copyright (c) 2016, Fuzhou Rockchip Electronics Co., Ltd
> + * Author: Lin Huang <hl@rock-chips.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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/devfreq-event.h>
> +#include <linux/kernel.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/list.h>
> +#include <linux/of.h>
> +
> +#define RK3399_DMC_NUM_CH	2
> +
> +/* DDRMON_CTRL */
> +#define DDRMON_CTRL	0x04
> +#define CLR_DDRMON_CTRL	(0x1f0000 << 0)
> +#define LPDDR4_EN	(0x10001 << 4)
> +#define HARDWARE_EN	(0x10001 << 3)
> +#define LPDDR3_EN	(0x10001 << 2)
> +#define SOFTWARE_EN	(0x10001 << 1)
> +#define TIME_CNT_EN	(0x10001 << 0)
> +
> +#define DDRMON_CH0_COUNT_NUM		0x28
> +#define DDRMON_CH0_DFI_ACCESS_NUM	0x2c
> +#define DDRMON_CH1_COUNT_NUM		0x3c
> +#define DDRMON_CH1_DFI_ACCESS_NUM	0x40
> +
> +/* pmu grf */
> +#define PMUGRF_OS_REG2	0x308
> +#define DDRTYPE_SHIFT	13
> +#define DDRTYPE_MASK	7
> +
> +enum {
> +	DDR3 = 3,
> +	LPDDR3 = 6,
> +	LPDDR4 = 7,
> +	UNUSED = 0xFF
> +};
> +
> +struct dmc_usage {
> +	u32 access;
> +	u32 total;
> +};
> +
> +struct rockchip_dfi {
> +	struct devfreq_event_dev *edev;
> +	struct devfreq_event_desc *desc;
> +	struct dmc_usage ch_usage[RK3399_DMC_NUM_CH];
> +	struct device *dev;
> +	void __iomem *regs;
> +	struct regmap *regmap_pmu;
> +	struct clk *clk;
> +};
> +
> +static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
> +{
> +	struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
> +	void __iomem *dfi_regs = info->regs;
> +	u32 val;
> +	u32 ddr_type;
> +
> +	/* get ddr type */
> +	regmap_read(info->regmap_pmu, PMUGRF_OS_REG2, &val);
> +	ddr_type = (val >> DDRTYPE_SHIFT) & DDRTYPE_MASK;
> +
> +	/* clear DDRMON_CTRL setting */
> +	writel_relaxed(CLR_DDRMON_CTRL, dfi_regs + DDRMON_CTRL);
> +
> +	/* set ddr type to dfi */
> +	if (ddr_type == LPDDR3)
> +		writel_relaxed(LPDDR3_EN, dfi_regs + DDRMON_CTRL);
> +	else if (ddr_type == LPDDR4)
> +		writel_relaxed(LPDDR4_EN, dfi_regs + DDRMON_CTRL);
> +
> +	/* enable count, use software mode */
> +	writel_relaxed(SOFTWARE_EN, dfi_regs + DDRMON_CTRL);
> +}
> +
> +static void rockchip_dfi_stop_hardware_counter(struct devfreq_event_dev *edev)
> +{
> +	struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
> +	void __iomem *dfi_regs = info->regs;
> +	u32 val;
> +
> +	val = readl_relaxed(dfi_regs + DDRMON_CTRL);
> +	val &= ~SOFTWARE_EN;
> +	writel_relaxed(val, dfi_regs + DDRMON_CTRL);
> +}
> +
> +static int rockchip_dfi_get_busier_ch(struct devfreq_event_dev *edev)
> +{
> +	struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
> +	u32 tmp, max = 0;
> +	u32 i, busier_ch = 0;
> +	void __iomem *dfi_regs = info->regs;
> +
> +	rockchip_dfi_stop_hardware_counter(edev);
> +
> +	/* Find out which channel is busier */
> +	for (i = 0; i < RK3399_DMC_NUM_CH; i++) {
> +		info->ch_usage[i].access = readl_relaxed(dfi_regs +
> +				DDRMON_CH0_DFI_ACCESS_NUM + i * 20);
> +		info->ch_usage[i].total = readl_relaxed(dfi_regs +
> +				DDRMON_CH0_COUNT_NUM + i * 20);
> +		tmp = info->ch_usage[i].access;
> +		if (tmp > max) {
> +			busier_ch = i;
> +			max = tmp;
> +		}
> +	}
> +	rockchip_dfi_start_hardware_counter(edev);
> +
> +	return busier_ch;
> +}
> +
> +static int rockchip_dfi_disable(struct devfreq_event_dev *edev)
> +{
> +	struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
> +
> +	rockchip_dfi_stop_hardware_counter(edev);
> +	clk_disable_unprepare(info->clk);
> +
> +	return 0;
> +}
> +
> +static int rockchip_dfi_enable(struct devfreq_event_dev *edev)
> +{
> +	struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
> +	int ret;
> +
> +	ret = clk_prepare_enable(info->clk);
> +	if (ret) {
> +		dev_err(&edev->dev, "failed to enable dfi clk: %d\n", ret);
> +		return ret;
> +	}
> +
> +	rockchip_dfi_start_hardware_counter(edev);
> +	return 0;
> +}
> +
> +static int rockchip_dfi_set_event(struct devfreq_event_dev *edev)
> +{
> +	return 0;
> +}
> +
> +static int rockchip_dfi_get_event(struct devfreq_event_dev *edev,
> +				  struct devfreq_event_data *edata)
> +{
> +	struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
> +	int busier_ch;
> +
> +	busier_ch = rockchip_dfi_get_busier_ch(edev);
> +
> +	edata->load_count = info->ch_usage[busier_ch].access;
> +	edata->total_count = info->ch_usage[busier_ch].total;
> +
> +	return 0;
> +}
> +
> +static const struct devfreq_event_ops rockchip_dfi_ops = {
> +	.disable = rockchip_dfi_disable,
> +	.enable = rockchip_dfi_enable,
> +	.get_event = rockchip_dfi_get_event,
> +	.set_event = rockchip_dfi_set_event,
> +};
> +
> +static const struct of_device_id rockchip_dfi_id_match[] = {
> +	{ .compatible = "rockchip,rk3399-dfi" },
> +	{ },
> +};
> +
> +static int rockchip_dfi_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct rockchip_dfi *data;
> +	struct resource *res;
> +	struct devfreq_event_desc *desc;
> +	struct device_node *np = pdev->dev.of_node, *node;
> +
> +	data = devm_kzalloc(dev, sizeof(struct rockchip_dfi), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	data->regs = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(data->regs))
> +		return PTR_ERR(data->regs);
> +
> +	data->clk = devm_clk_get(dev, "pclk_ddr_mon");
> +	if (IS_ERR(data->clk)) {
> +		dev_err(dev, "Cannot get the clk dmc_clk\n");
> +		return PTR_ERR(data->clk);
> +	};
> +
> +	/* try to find the optional reference to the pmu syscon */
> +	node = of_parse_phandle(np, "rockchip,pmu", 0);
> +	if (node) {
> +		data->regmap_pmu = syscon_node_to_regmap(node);
> +		if (IS_ERR(data->regmap_pmu))
> +			return PTR_ERR(data->regmap_pmu);
> +	}
> +	data->dev = dev;
> +
> +	desc = devm_kzalloc(dev, sizeof(*desc), GFP_KERNEL);
> +	if (!desc)
> +		return -ENOMEM;
> +
> +	desc->ops = &rockchip_dfi_ops;
> +	desc->driver_data = data;
> +	desc->name = np->name;
> +	data->desc = desc;
> +
> +	data->edev = devm_devfreq_event_add_edev(&pdev->dev, desc);
> +	if (IS_ERR(data->edev)) {
> +		dev_err(&pdev->dev,
> +			"failed to add devfreq-event device\n");
> +		return PTR_ERR(data->edev);
> +	}
> +
> +	platform_set_drvdata(pdev, data);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver rockchip_dfi_driver = {
> +	.probe	= rockchip_dfi_probe,
> +	.driver = {
> +		.name	= "rockchip-dfi",
> +		.of_match_table = rockchip_dfi_id_match,
> +	},
> +};
> +module_platform_driver(rockchip_dfi_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Lin Huang <hl@rock-chips.com>");
> +MODULE_DESCRIPTION("Rockchip dfi driver");
> 


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 5/7] PM / devfreq: event: support rockchip dfi controller

[Chanwoo Choi]

Hi Lin,

I add the one minor comment for full name of 'DRI'.

On 2016년 08월 01일 16:41, Chanwoo Choi wrote:
> Hi Lin,
> 
> Because you remove the 'RFC' prefix on patch title,
> I think that you better to make the documentation as following: 
> - Documentation/devicetree/bindings/devfreq/event/rockchip-dfi.txt
> 
> Regards,
> Chanwoo Choi
> 
> On 2016년 07월 29일 16:56, Lin Huang wrote:
>> on rk3399 platform, there is dfi conroller can monitor
>> ddr load, base on this result, we can do ddr freqency
>> scaling.
>>
>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>> Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
>> ---
>> Changes in v4:
>> - None
>>
>> Changes in v3:
>> - None
>>
>> Changes in v2:
>> - use clk_disable_unprepare and clk_enable_prepare
>> - remove clk_enable_prepare in probe
>> - remove rockchip_dfi_remove function
>>
>> Changes in v1:
>> - None
>>
>>  drivers/devfreq/event/Kconfig        |   7 +
>>  drivers/devfreq/event/Makefile       |   1 +
>>  drivers/devfreq/event/rockchip-dfi.c | 253 +++++++++++++++++++++++++++++++++++
>>  3 files changed, 261 insertions(+)
>>  create mode 100644 drivers/devfreq/event/rockchip-dfi.c
>>
>> diff --git a/drivers/devfreq/event/Kconfig b/drivers/devfreq/event/Kconfig
>> index a11720a..ff9279f 100644
>> --- a/drivers/devfreq/event/Kconfig
>> +++ b/drivers/devfreq/event/Kconfig
>> @@ -22,4 +22,11 @@ config DEVFREQ_EVENT_EXYNOS_PPMU
>>  	  (Platform Performance Monitoring Unit) counters to estimate the
>>  	  utilization of each module.
>>  
>> +config DEVFREQ_EVENT_ROCKCHIP_DFI
>> +	tristate "ROCKCHIP DFI DEVFREQ event Driver"
>> +	depends on ARCH_ROCKCHIP
>> +	help
>> +	  This add the devfreq-event driver for Rockchip SoC. It provides DFI
>> +	  (DDR Monitor Module) driver to count ddr load.

The DFI is "DDR Monitor Module" full name? I need the correct abbreviation 
and full name.

>> +
>>  endif # PM_DEVFREQ_EVENT
>> diff --git a/drivers/devfreq/event/Makefile b/drivers/devfreq/event/Makefile
>> index be146ea..e3f88fc 100644
>> --- a/drivers/devfreq/event/Makefile
>> +++ b/drivers/devfreq/event/Makefile
>> @@ -1,2 +1,3 @@
>>  # Exynos DEVFREQ Event Drivers
>>  obj-$(CONFIG_DEVFREQ_EVENT_EXYNOS_PPMU) += exynos-ppmu.o
>> +obj-$(CONFIG_DEVFREQ_EVENT_ROCKCHIP_DFI) += rockchip-dfi.o
>> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
>> new file mode 100644
>> index 0000000..96a0307
>> --- /dev/null
>> +++ b/drivers/devfreq/event/rockchip-dfi.c
>> @@ -0,0 +1,253 @@
>> +/*
>> + * Copyright (c) 2016, Fuzhou Rockchip Electronics Co., Ltd
>> + * Author: Lin Huang <hl@rock-chips.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope 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/devfreq-event.h>
>> +#include <linux/kernel.h>
>> +#include <linux/err.h>
>> +#include <linux/init.h>
>> +#include <linux/io.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +#include <linux/list.h>
>> +#include <linux/of.h>
>> +
>> +#define RK3399_DMC_NUM_CH	2
>> +
>> +/* DDRMON_CTRL */
>> +#define DDRMON_CTRL	0x04
>> +#define CLR_DDRMON_CTRL	(0x1f0000 << 0)
>> +#define LPDDR4_EN	(0x10001 << 4)
>> +#define HARDWARE_EN	(0x10001 << 3)
>> +#define LPDDR3_EN	(0x10001 << 2)
>> +#define SOFTWARE_EN	(0x10001 << 1)
>> +#define TIME_CNT_EN	(0x10001 << 0)
>> +
>> +#define DDRMON_CH0_COUNT_NUM		0x28
>> +#define DDRMON_CH0_DFI_ACCESS_NUM	0x2c
>> +#define DDRMON_CH1_COUNT_NUM		0x3c
>> +#define DDRMON_CH1_DFI_ACCESS_NUM	0x40
>> +
>> +/* pmu grf */
>> +#define PMUGRF_OS_REG2	0x308
>> +#define DDRTYPE_SHIFT	13
>> +#define DDRTYPE_MASK	7
>> +
>> +enum {
>> +	DDR3 = 3,
>> +	LPDDR3 = 6,
>> +	LPDDR4 = 7,
>> +	UNUSED = 0xFF
>> +};
>> +
>> +struct dmc_usage {
>> +	u32 access;
>> +	u32 total;
>> +};
>> +
>> +struct rockchip_dfi {
>> +	struct devfreq_event_dev *edev;
>> +	struct devfreq_event_desc *desc;
>> +	struct dmc_usage ch_usage[RK3399_DMC_NUM_CH];
>> +	struct device *dev;
>> +	void __iomem *regs;
>> +	struct regmap *regmap_pmu;
>> +	struct clk *clk;
>> +};
>> +
>> +static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
>> +{
>> +	struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
>> +	void __iomem *dfi_regs = info->regs;
>> +	u32 val;
>> +	u32 ddr_type;
>> +
>> +	/* get ddr type */
>> +	regmap_read(info->regmap_pmu, PMUGRF_OS_REG2, &val);
>> +	ddr_type = (val >> DDRTYPE_SHIFT) & DDRTYPE_MASK;
>> +
>> +	/* clear DDRMON_CTRL setting */
>> +	writel_relaxed(CLR_DDRMON_CTRL, dfi_regs + DDRMON_CTRL);
>> +
>> +	/* set ddr type to dfi */
>> +	if (ddr_type == LPDDR3)
>> +		writel_relaxed(LPDDR3_EN, dfi_regs + DDRMON_CTRL);
>> +	else if (ddr_type == LPDDR4)
>> +		writel_relaxed(LPDDR4_EN, dfi_regs + DDRMON_CTRL);
>> +
>> +	/* enable count, use software mode */
>> +	writel_relaxed(SOFTWARE_EN, dfi_regs + DDRMON_CTRL);
>> +}
>> +
>> +static void rockchip_dfi_stop_hardware_counter(struct devfreq_event_dev *edev)
>> +{
>> +	struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
>> +	void __iomem *dfi_regs = info->regs;
>> +	u32 val;
>> +
>> +	val = readl_relaxed(dfi_regs + DDRMON_CTRL);
>> +	val &= ~SOFTWARE_EN;
>> +	writel_relaxed(val, dfi_regs + DDRMON_CTRL);
>> +}
>> +
>> +static int rockchip_dfi_get_busier_ch(struct devfreq_event_dev *edev)
>> +{
>> +	struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
>> +	u32 tmp, max = 0;
>> +	u32 i, busier_ch = 0;
>> +	void __iomem *dfi_regs = info->regs;
>> +
>> +	rockchip_dfi_stop_hardware_counter(edev);
>> +
>> +	/* Find out which channel is busier */
>> +	for (i = 0; i < RK3399_DMC_NUM_CH; i++) {
>> +		info->ch_usage[i].access = readl_relaxed(dfi_regs +
>> +				DDRMON_CH0_DFI_ACCESS_NUM + i * 20);
>> +		info->ch_usage[i].total = readl_relaxed(dfi_regs +
>> +				DDRMON_CH0_COUNT_NUM + i * 20);
>> +		tmp = info->ch_usage[i].access;
>> +		if (tmp > max) {
>> +			busier_ch = i;
>> +			max = tmp;
>> +		}
>> +	}
>> +	rockchip_dfi_start_hardware_counter(edev);
>> +
>> +	return busier_ch;
>> +}
>> +
>> +static int rockchip_dfi_disable(struct devfreq_event_dev *edev)
>> +{
>> +	struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
>> +
>> +	rockchip_dfi_stop_hardware_counter(edev);
>> +	clk_disable_unprepare(info->clk);
>> +
>> +	return 0;
>> +}
>> +
>> +static int rockchip_dfi_enable(struct devfreq_event_dev *edev)
>> +{
>> +	struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
>> +	int ret;
>> +
>> +	ret = clk_prepare_enable(info->clk);
>> +	if (ret) {
>> +		dev_err(&edev->dev, "failed to enable dfi clk: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	rockchip_dfi_start_hardware_counter(edev);
>> +	return 0;
>> +}
>> +
>> +static int rockchip_dfi_set_event(struct devfreq_event_dev *edev)
>> +{
>> +	return 0;
>> +}
>> +
>> +static int rockchip_dfi_get_event(struct devfreq_event_dev *edev,
>> +				  struct devfreq_event_data *edata)
>> +{
>> +	struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
>> +	int busier_ch;
>> +
>> +	busier_ch = rockchip_dfi_get_busier_ch(edev);
>> +
>> +	edata->load_count = info->ch_usage[busier_ch].access;
>> +	edata->total_count = info->ch_usage[busier_ch].total;
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct devfreq_event_ops rockchip_dfi_ops = {
>> +	.disable = rockchip_dfi_disable,
>> +	.enable = rockchip_dfi_enable,
>> +	.get_event = rockchip_dfi_get_event,
>> +	.set_event = rockchip_dfi_set_event,
>> +};
>> +
>> +static const struct of_device_id rockchip_dfi_id_match[] = {
>> +	{ .compatible = "rockchip,rk3399-dfi" },
>> +	{ },
>> +};
>> +
>> +static int rockchip_dfi_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct rockchip_dfi *data;
>> +	struct resource *res;
>> +	struct devfreq_event_desc *desc;
>> +	struct device_node *np = pdev->dev.of_node, *node;
>> +
>> +	data = devm_kzalloc(dev, sizeof(struct rockchip_dfi), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	data->regs = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(data->regs))
>> +		return PTR_ERR(data->regs);
>> +
>> +	data->clk = devm_clk_get(dev, "pclk_ddr_mon");
>> +	if (IS_ERR(data->clk)) {
>> +		dev_err(dev, "Cannot get the clk dmc_clk\n");
>> +		return PTR_ERR(data->clk);
>> +	};
>> +
>> +	/* try to find the optional reference to the pmu syscon */
>> +	node = of_parse_phandle(np, "rockchip,pmu", 0);
>> +	if (node) {
>> +		data->regmap_pmu = syscon_node_to_regmap(node);
>> +		if (IS_ERR(data->regmap_pmu))
>> +			return PTR_ERR(data->regmap_pmu);
>> +	}
>> +	data->dev = dev;
>> +
>> +	desc = devm_kzalloc(dev, sizeof(*desc), GFP_KERNEL);
>> +	if (!desc)
>> +		return -ENOMEM;
>> +
>> +	desc->ops = &rockchip_dfi_ops;
>> +	desc->driver_data = data;
>> +	desc->name = np->name;
>> +	data->desc = desc;
>> +
>> +	data->edev = devm_devfreq_event_add_edev(&pdev->dev, desc);
>> +	if (IS_ERR(data->edev)) {
>> +		dev_err(&pdev->dev,
>> +			"failed to add devfreq-event device\n");
>> +		return PTR_ERR(data->edev);
>> +	}
>> +
>> +	platform_set_drvdata(pdev, data);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver rockchip_dfi_driver = {
>> +	.probe	= rockchip_dfi_probe,
>> +	.driver = {
>> +		.name	= "rockchip-dfi",
>> +		.of_match_table = rockchip_dfi_id_match,
>> +	},
>> +};
>> +module_platform_driver(rockchip_dfi_driver);
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_AUTHOR("Lin Huang <hl@rock-chips.com>");
>> +MODULE_DESCRIPTION("Rockchip dfi driver");

s/dfi -> DFI

Regards,
Chanwoo Choi


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 5/7] PM / devfreq: event: support rockchip dfi controller

[hl]

Hi Chanwoo Choi,

On 2016年08月01日 16:08, Chanwoo Choi wrote:
> Hi Lin,
>
> I add the one minor comment for full name of 'DRI'.
>
> On 2016년 08월 01일 16:41, Chanwoo Choi wrote:
>> Hi Lin,
>>
>> Because you remove the 'RFC' prefix on patch title,
>> I think that you better to make the documentation as following:
>> - Documentation/devicetree/bindings/devfreq/event/rockchip-dfi.txt
>>
>> Regards,
>> Chanwoo Choi
>>
>> On 2016년 07월 29일 16:56, Lin Huang wrote:
>>> on rk3399 platform, there is dfi conroller can monitor
>>> ddr load, base on this result, we can do ddr freqency
>>> scaling.
>>>
>>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>>> Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
>>> ---
>>> Changes in v4:
>>> - None
>>>
>>> Changes in v3:
>>> - None
>>>
>>> Changes in v2:
>>> - use clk_disable_unprepare and clk_enable_prepare
>>> - remove clk_enable_prepare in probe
>>> - remove rockchip_dfi_remove function
>>>
>>> Changes in v1:
>>> - None
>>>
>>>   drivers/devfreq/event/Kconfig        |   7 +
>>>   drivers/devfreq/event/Makefile       |   1 +
>>>   drivers/devfreq/event/rockchip-dfi.c | 253 +++++++++++++++++++++++++++++++++++
>>>   3 files changed, 261 insertions(+)
>>>   create mode 100644 drivers/devfreq/event/rockchip-dfi.c
>>>
>>> diff --git a/drivers/devfreq/event/Kconfig b/drivers/devfreq/event/Kconfig
>>> index a11720a..ff9279f 100644
>>> --- a/drivers/devfreq/event/Kconfig
>>> +++ b/drivers/devfreq/event/Kconfig
>>> @@ -22,4 +22,11 @@ config DEVFREQ_EVENT_EXYNOS_PPMU
>>>   	  (Platform Performance Monitoring Unit) counters to estimate the
>>>   	  utilization of each module.
>>>   
>>> +config DEVFREQ_EVENT_ROCKCHIP_DFI
>>> +	tristate "ROCKCHIP DFI DEVFREQ event Driver"
>>> +	depends on ARCH_ROCKCHIP
>>> +	help
>>> +	  This add the devfreq-event driver for Rockchip SoC. It provides DFI
>>> +	  (DDR Monitor Module) driver to count ddr load.
> The DFI is "DDR Monitor Module" full name? I need the correct abbreviation
> and full name.
We just call this module DFI in datasheet, and this module function is 
ddr monitor module,
yes, it is do not fit the full name, but i think it is better follow the 
datasheet name.
>
>>> +
>>>   endif # PM_DEVFREQ_EVENT
>>> diff --git a/drivers/devfreq/event/Makefile b/drivers/devfreq/event/Makefile
>>> index be146ea..e3f88fc 100644
>>> --- a/drivers/devfreq/event/Makefile
>>> +++ b/drivers/devfreq/event/Makefile
>>> @@ -1,2 +1,3 @@
>>>   # Exynos DEVFREQ Event Drivers
>>>   obj-$(CONFIG_DEVFREQ_EVENT_EXYNOS_PPMU) += exynos-ppmu.o
>>> +obj-$(CONFIG_DEVFREQ_EVENT_ROCKCHIP_DFI) += rockchip-dfi.o
>>> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
>>> new file mode 100644
>>> index 0000000..96a0307
>>> --- /dev/null
>>> +++ b/drivers/devfreq/event/rockchip-dfi.c
>>> @@ -0,0 +1,253 @@
>>> +/*
>>> + * Copyright (c) 2016, Fuzhou Rockchip Electronics Co., Ltd
>>> + * Author: Lin Huang <hl@rock-chips.com>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify it
>>> + * under the terms and conditions of the GNU General Public License,
>>> + * version 2, as published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope 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/devfreq-event.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/err.h>
>>> +#include <linux/init.h>
>>> +#include <linux/io.h>
>>> +#include <linux/mfd/syscon.h>
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/list.h>
>>> +#include <linux/of.h>
>>> +
>>> +#define RK3399_DMC_NUM_CH	2
>>> +
>>> +/* DDRMON_CTRL */
>>> +#define DDRMON_CTRL	0x04
>>> +#define CLR_DDRMON_CTRL	(0x1f0000 << 0)
>>> +#define LPDDR4_EN	(0x10001 << 4)
>>> +#define HARDWARE_EN	(0x10001 << 3)
>>> +#define LPDDR3_EN	(0x10001 << 2)
>>> +#define SOFTWARE_EN	(0x10001 << 1)
>>> +#define TIME_CNT_EN	(0x10001 << 0)
>>> +
>>> +#define DDRMON_CH0_COUNT_NUM		0x28
>>> +#define DDRMON_CH0_DFI_ACCESS_NUM	0x2c
>>> +#define DDRMON_CH1_COUNT_NUM		0x3c
>>> +#define DDRMON_CH1_DFI_ACCESS_NUM	0x40
>>> +
>>> +/* pmu grf */
>>> +#define PMUGRF_OS_REG2	0x308
>>> +#define DDRTYPE_SHIFT	13
>>> +#define DDRTYPE_MASK	7
>>> +
>>> +enum {
>>> +	DDR3 = 3,
>>> +	LPDDR3 = 6,
>>> +	LPDDR4 = 7,
>>> +	UNUSED = 0xFF
>>> +};
>>> +
>>> +struct dmc_usage {
>>> +	u32 access;
>>> +	u32 total;
>>> +};
>>> +
>>> +struct rockchip_dfi {
>>> +	struct devfreq_event_dev *edev;
>>> +	struct devfreq_event_desc *desc;
>>> +	struct dmc_usage ch_usage[RK3399_DMC_NUM_CH];
>>> +	struct device *dev;
>>> +	void __iomem *regs;
>>> +	struct regmap *regmap_pmu;
>>> +	struct clk *clk;
>>> +};
>>> +
>>> +static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
>>> +{
>>> +	struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
>>> +	void __iomem *dfi_regs = info->regs;
>>> +	u32 val;
>>> +	u32 ddr_type;
>>> +
>>> +	/* get ddr type */
>>> +	regmap_read(info->regmap_pmu, PMUGRF_OS_REG2, &val);
>>> +	ddr_type = (val >> DDRTYPE_SHIFT) & DDRTYPE_MASK;
>>> +
>>> +	/* clear DDRMON_CTRL setting */
>>> +	writel_relaxed(CLR_DDRMON_CTRL, dfi_regs + DDRMON_CTRL);
>>> +
>>> +	/* set ddr type to dfi */
>>> +	if (ddr_type == LPDDR3)
>>> +		writel_relaxed(LPDDR3_EN, dfi_regs + DDRMON_CTRL);
>>> +	else if (ddr_type == LPDDR4)
>>> +		writel_relaxed(LPDDR4_EN, dfi_regs + DDRMON_CTRL);
>>> +
>>> +	/* enable count, use software mode */
>>> +	writel_relaxed(SOFTWARE_EN, dfi_regs + DDRMON_CTRL);
>>> +}
>>> +
>>> +static void rockchip_dfi_stop_hardware_counter(struct devfreq_event_dev *edev)
>>> +{
>>> +	struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
>>> +	void __iomem *dfi_regs = info->regs;
>>> +	u32 val;
>>> +
>>> +	val = readl_relaxed(dfi_regs + DDRMON_CTRL);
>>> +	val &= ~SOFTWARE_EN;
>>> +	writel_relaxed(val, dfi_regs + DDRMON_CTRL);
>>> +}
>>> +
>>> +static int rockchip_dfi_get_busier_ch(struct devfreq_event_dev *edev)
>>> +{
>>> +	struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
>>> +	u32 tmp, max = 0;
>>> +	u32 i, busier_ch = 0;
>>> +	void __iomem *dfi_regs = info->regs;
>>> +
>>> +	rockchip_dfi_stop_hardware_counter(edev);
>>> +
>>> +	/* Find out which channel is busier */
>>> +	for (i = 0; i < RK3399_DMC_NUM_CH; i++) {
>>> +		info->ch_usage[i].access = readl_relaxed(dfi_regs +
>>> +				DDRMON_CH0_DFI_ACCESS_NUM + i * 20);
>>> +		info->ch_usage[i].total = readl_relaxed(dfi_regs +
>>> +				DDRMON_CH0_COUNT_NUM + i * 20);
>>> +		tmp = info->ch_usage[i].access;
>>> +		if (tmp > max) {
>>> +			busier_ch = i;
>>> +			max = tmp;
>>> +		}
>>> +	}
>>> +	rockchip_dfi_start_hardware_counter(edev);
>>> +
>>> +	return busier_ch;
>>> +}
>>> +
>>> +static int rockchip_dfi_disable(struct devfreq_event_dev *edev)
>>> +{
>>> +	struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
>>> +
>>> +	rockchip_dfi_stop_hardware_counter(edev);
>>> +	clk_disable_unprepare(info->clk);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int rockchip_dfi_enable(struct devfreq_event_dev *edev)
>>> +{
>>> +	struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
>>> +	int ret;
>>> +
>>> +	ret = clk_prepare_enable(info->clk);
>>> +	if (ret) {
>>> +		dev_err(&edev->dev, "failed to enable dfi clk: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	rockchip_dfi_start_hardware_counter(edev);
>>> +	return 0;
>>> +}
>>> +
>>> +static int rockchip_dfi_set_event(struct devfreq_event_dev *edev)
>>> +{
>>> +	return 0;
>>> +}
>>> +
>>> +static int rockchip_dfi_get_event(struct devfreq_event_dev *edev,
>>> +				  struct devfreq_event_data *edata)
>>> +{
>>> +	struct rockchip_dfi *info = devfreq_event_get_drvdata(edev);
>>> +	int busier_ch;
>>> +
>>> +	busier_ch = rockchip_dfi_get_busier_ch(edev);
>>> +
>>> +	edata->load_count = info->ch_usage[busier_ch].access;
>>> +	edata->total_count = info->ch_usage[busier_ch].total;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct devfreq_event_ops rockchip_dfi_ops = {
>>> +	.disable = rockchip_dfi_disable,
>>> +	.enable = rockchip_dfi_enable,
>>> +	.get_event = rockchip_dfi_get_event,
>>> +	.set_event = rockchip_dfi_set_event,
>>> +};
>>> +
>>> +static const struct of_device_id rockchip_dfi_id_match[] = {
>>> +	{ .compatible = "rockchip,rk3399-dfi" },
>>> +	{ },
>>> +};
>>> +
>>> +static int rockchip_dfi_probe(struct platform_device *pdev)
>>> +{
>>> +	struct device *dev = &pdev->dev;
>>> +	struct rockchip_dfi *data;
>>> +	struct resource *res;
>>> +	struct devfreq_event_desc *desc;
>>> +	struct device_node *np = pdev->dev.of_node, *node;
>>> +
>>> +	data = devm_kzalloc(dev, sizeof(struct rockchip_dfi), GFP_KERNEL);
>>> +	if (!data)
>>> +		return -ENOMEM;
>>> +
>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +	data->regs = devm_ioremap_resource(&pdev->dev, res);
>>> +	if (IS_ERR(data->regs))
>>> +		return PTR_ERR(data->regs);
>>> +
>>> +	data->clk = devm_clk_get(dev, "pclk_ddr_mon");
>>> +	if (IS_ERR(data->clk)) {
>>> +		dev_err(dev, "Cannot get the clk dmc_clk\n");
>>> +		return PTR_ERR(data->clk);
>>> +	};
>>> +
>>> +	/* try to find the optional reference to the pmu syscon */
>>> +	node = of_parse_phandle(np, "rockchip,pmu", 0);
>>> +	if (node) {
>>> +		data->regmap_pmu = syscon_node_to_regmap(node);
>>> +		if (IS_ERR(data->regmap_pmu))
>>> +			return PTR_ERR(data->regmap_pmu);
>>> +	}
>>> +	data->dev = dev;
>>> +
>>> +	desc = devm_kzalloc(dev, sizeof(*desc), GFP_KERNEL);
>>> +	if (!desc)
>>> +		return -ENOMEM;
>>> +
>>> +	desc->ops = &rockchip_dfi_ops;
>>> +	desc->driver_data = data;
>>> +	desc->name = np->name;
>>> +	data->desc = desc;
>>> +
>>> +	data->edev = devm_devfreq_event_add_edev(&pdev->dev, desc);
>>> +	if (IS_ERR(data->edev)) {
>>> +		dev_err(&pdev->dev,
>>> +			"failed to add devfreq-event device\n");
>>> +		return PTR_ERR(data->edev);
>>> +	}
>>> +
>>> +	platform_set_drvdata(pdev, data);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static struct platform_driver rockchip_dfi_driver = {
>>> +	.probe	= rockchip_dfi_probe,
>>> +	.driver = {
>>> +		.name	= "rockchip-dfi",
>>> +		.of_match_table = rockchip_dfi_id_match,
>>> +	},
>>> +};
>>> +module_platform_driver(rockchip_dfi_driver);
>>> +
>>> +MODULE_LICENSE("GPL v2");
>>> +MODULE_AUTHOR("Lin Huang <hl@rock-chips.com>");
>>> +MODULE_DESCRIPTION("Rockchip dfi driver");
> s/dfi -> DFI
>
> Regards,
> Chanwoo Choi
>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

-- 
Lin Huang

Re: [PATCH v4 5/7] PM / devfreq: event: support rockchip dfi controller

[Chanwoo Choi]

Hi Lin,

On 2016년 08월 01일 17:27, hl wrote:
> Hi Chanwoo Choi,
> 
> On 2016年08月01日 16:08, Chanwoo Choi wrote:
>> Hi Lin,
>>
>> I add the one minor comment for full name of 'DRI'.
>>
>> On 2016년 08월 01일 16:41, Chanwoo Choi wrote:
>>> Hi Lin,
>>>
>>> Because you remove the 'RFC' prefix on patch title,
>>> I think that you better to make the documentation as following:
>>> - Documentation/devicetree/bindings/devfreq/event/rockchip-dfi.txt
>>>
>>> Regards,
>>> Chanwoo Choi
>>>
>>> On 2016년 07월 29일 16:56, Lin Huang wrote:
>>>> on rk3399 platform, there is dfi conroller can monitor
>>>> ddr load, base on this result, we can do ddr freqency
>>>> scaling.
>>>>
>>>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>>>> Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>> ---
>>>> Changes in v4:
>>>> - None
>>>>
>>>> Changes in v3:
>>>> - None
>>>>
>>>> Changes in v2:
>>>> - use clk_disable_unprepare and clk_enable_prepare
>>>> - remove clk_enable_prepare in probe
>>>> - remove rockchip_dfi_remove function
>>>>
>>>> Changes in v1:
>>>> - None
>>>>
>>>>   drivers/devfreq/event/Kconfig        |   7 +
>>>>   drivers/devfreq/event/Makefile       |   1 +
>>>>   drivers/devfreq/event/rockchip-dfi.c | 253 +++++++++++++++++++++++++++++++++++
>>>>   3 files changed, 261 insertions(+)
>>>>   create mode 100644 drivers/devfreq/event/rockchip-dfi.c
>>>>
>>>> diff --git a/drivers/devfreq/event/Kconfig b/drivers/devfreq/event/Kconfig
>>>> index a11720a..ff9279f 100644
>>>> --- a/drivers/devfreq/event/Kconfig
>>>> +++ b/drivers/devfreq/event/Kconfig
>>>> @@ -22,4 +22,11 @@ config DEVFREQ_EVENT_EXYNOS_PPMU
>>>>         (Platform Performance Monitoring Unit) counters to estimate the
>>>>         utilization of each module.
>>>>   +config DEVFREQ_EVENT_ROCKCHIP_DFI
>>>> +    tristate "ROCKCHIP DFI DEVFREQ event Driver"
>>>> +    depends on ARCH_ROCKCHIP
>>>> +    help
>>>> +      This add the devfreq-event driver for Rockchip SoC. It provides DFI
>>>> +      (DDR Monitor Module) driver to count ddr load.
>> The DFI is "DDR Monitor Module" full name? I need the correct abbreviation
>> and full name.
> We just call this module DFI in datasheet, and this module function is ddr monitor module,
> yes, it is do not fit the full name, but i think it is better follow the datasheet name.

Are there any full name of DFI? If the people who don't know the detailed history about
this workd (DFI), it causes the confusion and they don't understand it.

I think that if you want to use the DFI work, you need to add the more detailed description
why DFI word is used.

>>
>>>> +
>>>>   endif # PM_DEVFREQ_EVENT
>>>> diff --git a/drivers/devfreq/event/Makefile b/drivers/devfreq/event/Makefile
>>>> index be146ea..e3f88fc 100644
>>>> --- a/drivers/devfreq/event/Makefile
>>>> +++ b/drivers/devfreq/event/Makefile
>>>> @@ -1,2 +1,3 @@
>>>>   # Exynos DEVFREQ Event Drivers
>>>>   obj-$(CONFIG_DEVFREQ_EVENT_EXYNOS_PPMU) += exynos-ppmu.o
>>>> +obj-$(CONFIG_DEVFREQ_EVENT_ROCKCHIP_DFI) += rockchip-dfi.o
>>>> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
>>>> new file mode 100644
>>>> index 0000000..96a0307
>>>> --- /dev/null
>>>> +++ b/drivers/devfreq/event/rockchip-dfi.c
>>>> @@ -0,0 +1,253 @@
>>>> +/*
>>>> + * Copyright (c) 2016, Fuzhou Rockchip Electronics Co., Ltd
>>>> + * Author: Lin Huang <hl@rock-chips.com>
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify it
>>>> + * under the terms and conditions of the GNU General Public License,
>>>> + * version 2, as published by the Free Software Foundation.
>>>> + *
>>>> + * This program is distributed in the hope 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/devfreq-event.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/err.h>
>>>> +#include <linux/init.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/mfd/syscon.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/regmap.h>
>>>> +#include <linux/slab.h>
>>>> +#include <linux/list.h>
>>>> +#include <linux/of.h>
>>>> +
>>>> +#define RK3399_DMC_NUM_CH    2
>>>> +
>>>> +/* DDRMON_CTRL */
>>>> +#define DDRMON_CTRL    0x04
>>>> +#define CLR_DDRMON_CTRL    (0x1f0000 << 0)
>>>> +#define LPDDR4_EN    (0x10001 << 4)
>>>> +#define HARDWARE_EN    (0x10001 << 3)
>>>> +#define LPDDR3_EN    (0x10001 << 2)
>>>> +#define SOFTWARE_EN    (0x10001 << 1)
>>>> +#define TIME_CNT_EN    (0x10001 << 0)

[snip]

>>>> +};
>>>> +module_platform_driver(rockchip_dfi_driver);
>>>> +
>>>> +MODULE_LICENSE("GPL v2");
>>>> +MODULE_AUTHOR("Lin Huang <hl@rock-chips.com>");
>>>> +MODULE_DESCRIPTION("Rockchip dfi driver");
>> s/dfi -> DFI

You should use the capital letter.

Regards,
Chanwoo Choi

[PATCH v4 6/7] PM / devfreq: rockchip: add devfreq driver for rk3399 dmc

[Lin Huang]

base on dfi result, we do ddr frequency scaling, register
dmc driver to devfreq framework, and use simple-ondemand
policy.

Signed-off-by: Lin Huang <hl@rock-chips.com>
---
Changes in v4:
- use arm_smccc_smc() function talk to bl31
- delete rockchip_dmc.c file and config
- delete dmc_notify
- adjust probe order
 
Changes in v3:
- operate dram setting through sip call
- imporve set rate flow

Changes in v2:
- None
 
Changes in v1:
- move dfi controller to event
- fix set voltage sequence when set rate fail
- change Kconfig type from tristate to bool
- move unuse EXPORT_SYMBOL_GPL()

 drivers/devfreq/Kconfig               |   1 +
 drivers/devfreq/Makefile              |   1 +
 drivers/devfreq/rockchip/Kconfig      |   8 +
 drivers/devfreq/rockchip/Makefile     |   1 +
 drivers/devfreq/rockchip/rk3399_dmc.c | 473 ++++++++++++++++++++++++++++++++++
 5 files changed, 484 insertions(+)
 create mode 100644 drivers/devfreq/rockchip/Kconfig
 create mode 100644 drivers/devfreq/rockchip/Makefile
 create mode 100644 drivers/devfreq/rockchip/rk3399_dmc.c

diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index 64281bb..acb2a57 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -99,5 +99,6 @@ config ARM_TEGRA_DEVFREQ
          operating frequencies and voltages with OPP support.
 
 source "drivers/devfreq/event/Kconfig"
+source "drivers/devfreq/rockchip/Kconfig"
 
 endif # PM_DEVFREQ
diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
index 5134f9e..d844e23 100644
--- a/drivers/devfreq/Makefile
+++ b/drivers/devfreq/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_DEVFREQ_GOV_USERSPACE)	+= governor_userspace.o
 obj-$(CONFIG_ARM_EXYNOS4_BUS_DEVFREQ)	+= exynos/
 obj-$(CONFIG_ARM_EXYNOS5_BUS_DEVFREQ)	+= exynos/
 obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra-devfreq.o
+obj-$(CONFIG_ARCH_ROCKCHIP)		+= rockchip/
 
 # DEVFREQ Event Drivers
 obj-$(CONFIG_PM_DEVFREQ_EVENT)		+= event/
diff --git a/drivers/devfreq/rockchip/Kconfig b/drivers/devfreq/rockchip/Kconfig
new file mode 100644
index 0000000..d8f9e66
--- /dev/null
+++ b/drivers/devfreq/rockchip/Kconfig
@@ -0,0 +1,8 @@
+config ARM_RK3399_DMC_DEVFREQ
+	tristate "ARM RK3399 DMC DEVFREQ Driver"
+	select PM_OPP
+	select DEVFREQ_GOV_SIMPLE_ONDEMAND
+	help
+          This adds the DEVFREQ driver for the RK3399 dmc. It sets the frequency
+          for the memory controller and reads the usage counts from hardware.
+
diff --git a/drivers/devfreq/rockchip/Makefile b/drivers/devfreq/rockchip/Makefile
new file mode 100644
index 0000000..c62c105
--- /dev/null
+++ b/drivers/devfreq/rockchip/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
diff --git a/drivers/devfreq/rockchip/rk3399_dmc.c b/drivers/devfreq/rockchip/rk3399_dmc.c
new file mode 100644
index 0000000..527aa11
--- /dev/null
+++ b/drivers/devfreq/rockchip/rk3399_dmc.c
@@ -0,0 +1,473 @@
+/*
+ * Copyright (c) 2016, Fuzhou Rockchip Electronics Co., Ltd
+ * Author: Lin Huang <hl@rock-chips.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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/arm-smccc.h>
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/devfreq.h>
+#include <linux/devfreq-event.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_opp.h>
+#include <linux/regulator/consumer.h>
+#include <linux/rwsem.h>
+#include <linux/suspend.h>
+#include <linux/syscore_ops.h>
+
+#include <soc/rockchip/rockchip_sip.h>
+
+struct dram_timing {
+	unsigned int ddr3_speed_bin;
+	unsigned int pd_idle;
+	unsigned int sr_idle;
+	unsigned int sr_mc_gate_idle;
+	unsigned int srpd_lite_idle;
+	unsigned int standby_idle;
+	unsigned int dram_dll_dis_freq;
+	unsigned int phy_dll_dis_freq;
+	unsigned int ddr3_odt_dis_freq;
+	unsigned int ddr3_drv;
+	unsigned int ddr3_odt;
+	unsigned int phy_ddr3_ca_drv;
+	unsigned int phy_ddr3_dq_drv;
+	unsigned int phy_ddr3_odt;
+	unsigned int lpddr3_odt_dis_freq;
+	unsigned int lpddr3_drv;
+	unsigned int lpddr3_odt;
+	unsigned int phy_lpddr3_ca_drv;
+	unsigned int phy_lpddr3_dq_drv;
+	unsigned int phy_lpddr3_odt;
+	unsigned int lpddr4_odt_dis_freq;
+	unsigned int lpddr4_drv;
+	unsigned int lpddr4_dq_odt;
+	unsigned int lpddr4_ca_odt;
+	unsigned int phy_lpddr4_ca_drv;
+	unsigned int phy_lpddr4_ck_cs_drv;
+	unsigned int phy_lpddr4_dq_drv;
+	unsigned int phy_lpddr4_odt;
+};
+
+struct rk3399_dmcfreq {
+	struct device *dev;
+	struct devfreq *devfreq;
+	struct devfreq_simple_ondemand_data ondemand_data;
+	struct clk *dmc_clk;
+	struct devfreq_event_dev *edev;
+	struct mutex lock;
+	struct dram_timing *timing;
+	wait_queue_head_t	wait_dcf_queue;
+	int irq;
+	int wait_dcf_flag;
+	struct regulator *vdd_center;
+	unsigned long rate, target_rate;
+	unsigned long volt, target_volt;
+};
+
+static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
+				 u32 flags)
+{
+	struct platform_device *pdev = container_of(dev, struct platform_device,
+						    dev);
+	struct rk3399_dmcfreq *dmcfreq = platform_get_drvdata(pdev);
+	struct dev_pm_opp *opp;
+	unsigned long old_clk_rate = dmcfreq->rate;
+	unsigned long target_volt, target_rate;
+	int err;
+
+	rcu_read_lock();
+	opp = devfreq_recommended_opp(dev, freq, flags);
+	if (IS_ERR(opp)) {
+		rcu_read_unlock();
+		return PTR_ERR(opp);
+	}
+
+	target_rate = dev_pm_opp_get_freq(opp);
+	target_volt = dev_pm_opp_get_voltage(opp);
+	opp = devfreq_recommended_opp(dev, &dmcfreq->rate, flags);
+	if (IS_ERR(opp)) {
+		rcu_read_unlock();
+		return PTR_ERR(opp);
+	}
+	dmcfreq->volt = dev_pm_opp_get_voltage(opp);
+	rcu_read_unlock();
+
+	if (dmcfreq->rate == target_rate)
+		return 0;
+
+	mutex_lock(&dmcfreq->lock);
+
+	/*
+	 * if frequency scaling from low to high, adjust voltage first;
+	 * if frequency scaling from high to low, adjuset frequency first;
+	 */
+	if (old_clk_rate < target_rate) {
+		err = regulator_set_voltage(dmcfreq->vdd_center, target_volt,
+					    target_volt);
+		if (err) {
+			dev_err(dev, "Unable to set vol %lu\n", target_volt);
+			goto out;
+		}
+	}
+	dmcfreq->wait_dcf_flag = 1;
+	err = clk_set_rate(dmcfreq->dmc_clk, target_rate);
+	if (err) {
+		dev_err(dev,
+			"Unable to set freq %lu. Current freq %lu. Error %d\n",
+			target_rate, old_clk_rate, err);
+		regulator_set_voltage(dmcfreq->vdd_center, dmcfreq->volt,
+				      dmcfreq->volt);
+		goto out;
+	}
+
+	/*
+	 * wait until bcf irq happen, it means freq scaling finish in bl31,
+	 * use 100ms as timeout time
+	 */
+	if (!wait_event_timeout(dmcfreq->wait_dcf_queue,
+				!dmcfreq->wait_dcf_flag, HZ / 10))
+		dev_warn(dev, "Timeout waiting for dcf irq\n");
+	/*
+	 * check the dpll rate
+	 * there only two result we will get,
+	 * 1. ddr frequency scaling fail, we still get the old rate
+	 * 2, ddr frequency scaling sucessful, we get the rate we set
+	 */
+	dmcfreq->rate = clk_get_rate(dmcfreq->dmc_clk);
+
+	/* if get the incorrect rate, set voltage to old value */
+	if (dmcfreq->rate != target_rate) {
+		dev_err(dev, "get wrong ddr frequency, Request freq %lu,\
+			Current freq %lu\n", target_rate, dmcfreq->rate);
+		regulator_set_voltage(dmcfreq->vdd_center, dmcfreq->volt,
+				      dmcfreq->volt);
+	} else if (old_clk_rate > target_rate)
+		err = regulator_set_voltage(dmcfreq->vdd_center, target_volt,
+					    target_volt);
+	if (err)
+		dev_err(dev, "Unable to set vol %lu\n", target_volt);
+
+out:
+	mutex_unlock(&dmcfreq->lock);
+	return err;
+}
+
+static int rk3399_dmcfreq_get_dev_status(struct device *dev,
+					 struct devfreq_dev_status *stat)
+{
+	struct platform_device *pdev = container_of(dev, struct platform_device,
+						    dev);
+	struct rk3399_dmcfreq *dmcfreq = platform_get_drvdata(pdev);
+	struct devfreq_event_data edata;
+
+	devfreq_event_get_event(dmcfreq->edev, &edata);
+
+	stat->current_frequency = dmcfreq->rate;
+	stat->busy_time = edata.load_count;
+	stat->total_time = edata.total_count;
+
+	return 0;
+}
+
+static int rk3399_dmcfreq_get_cur_freq(struct device *dev, unsigned long *freq)
+{
+	struct platform_device *pdev = container_of(dev, struct platform_device,
+						    dev);
+	struct rk3399_dmcfreq *dmcfreq = platform_get_drvdata(pdev);
+
+	*freq = dmcfreq->rate;
+
+	return 0;
+}
+
+static void rk3399_dmcfreq_exit(struct device *dev)
+{
+	struct platform_device *pdev = container_of(dev,
+						    struct platform_device,
+						    dev);
+	struct rk3399_dmcfreq *dmcfreq = platform_get_drvdata(pdev);
+
+	devfreq_unregister_opp_notifier(dev, dmcfreq->devfreq);
+}
+
+static struct devfreq_dev_profile rk3399_devfreq_dmc_profile = {
+	.polling_ms	= 200,
+	.target		= rk3399_dmcfreq_target,
+	.get_dev_status	= rk3399_dmcfreq_get_dev_status,
+	.get_cur_freq	= rk3399_dmcfreq_get_cur_freq,
+	.exit		= rk3399_dmcfreq_exit,
+};
+
+static __maybe_unused int rk3399_dmcfreq_suspend(struct device *dev)
+{
+	struct platform_device *pdev = container_of(dev,
+						    struct platform_device,
+						    dev);
+	struct rk3399_dmcfreq *dmcfreq = platform_get_drvdata(pdev);
+
+	devfreq_event_disable_edev(dmcfreq->edev);
+	devfreq_suspend_device(dmcfreq->devfreq);
+
+	return 0;
+}
+
+static __maybe_unused int rk3399_dmcfreq_resume(struct device *dev)
+{
+	struct platform_device *pdev = container_of(dev,
+						    struct platform_device,
+						    dev);
+	struct rk3399_dmcfreq *dmcfreq = platform_get_drvdata(pdev);
+
+	devfreq_event_enable_edev(dmcfreq->edev);
+	devfreq_resume_device(dmcfreq->devfreq);
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(rk3399_dmcfreq_pm, rk3399_dmcfreq_suspend,
+			 rk3399_dmcfreq_resume);
+
+static irqreturn_t rk3399_dmc_irq(int irq, void *dev_id)
+{
+	struct rk3399_dmcfreq *dmcfreq = dev_id;
+	struct arm_smccc_res res;
+
+	dmcfreq->wait_dcf_flag = 0;
+	wake_up(&dmcfreq->wait_dcf_queue);
+
+	/* clr dcf irq */
+	arm_smccc_smc(SIP_DDR_FREQ, 0, 0, CONFIG_DRAM_CLR_IRQ,
+		      0, 0, 0, 0, &res);
+
+	return IRQ_HANDLED;
+}
+
+static int of_do_get_timing(struct device_node *np,
+		struct dram_timing *timing)
+{
+	int ret;
+
+	ret = of_property_read_u32(np, "ddr3_speed_bin",
+				   &timing->ddr3_speed_bin);
+	ret |= of_property_read_u32(np, "pd_idle", &timing->pd_idle);
+	ret |= of_property_read_u32(np, "sr_idle", &timing->sr_idle);
+	ret |= of_property_read_u32(np, "sr_mc_gate_idle",
+				    &timing->sr_mc_gate_idle);
+	ret |= of_property_read_u32(np, "srpd_lite_idle",
+				    &timing->srpd_lite_idle);
+	ret |= of_property_read_u32(np, "standby_idle", &timing->standby_idle);
+	ret |= of_property_read_u32(np, "dram_dll_dis_freq",
+				    &timing->dram_dll_dis_freq);
+	ret |= of_property_read_u32(np, "phy_dll_dis_freq",
+				    &timing->phy_dll_dis_freq);
+	ret |= of_property_read_u32(np, "ddr3_odt_dis_freq",
+				    &timing->ddr3_odt_dis_freq);
+	ret |= of_property_read_u32(np, "ddr3_drv", &timing->ddr3_drv);
+	ret |= of_property_read_u32(np, "ddr3_odt", &timing->ddr3_odt);
+	ret |= of_property_read_u32(np, "phy_ddr3_ca_drv",
+				    &timing->phy_ddr3_ca_drv);
+	ret |= of_property_read_u32(np, "phy_ddr3_dq_drv",
+				    &timing->phy_ddr3_dq_drv);
+	ret |= of_property_read_u32(np, "phy_ddr3_odt", &timing->phy_ddr3_odt);
+	ret |= of_property_read_u32(np, "lpddr3_odt_dis_freq",
+				    &timing->lpddr3_odt_dis_freq);
+	ret |= of_property_read_u32(np, "lpddr3_drv", &timing->lpddr3_drv);
+	ret |= of_property_read_u32(np, "lpddr3_odt", &timing->lpddr3_odt);
+	ret |= of_property_read_u32(np, "phy_lpddr3_ca_drv",
+				    &timing->phy_lpddr3_ca_drv);
+	ret |= of_property_read_u32(np, "phy_lpddr3_dq_drv",
+				    &timing->phy_lpddr3_dq_drv);
+	ret |= of_property_read_u32(np, "phy_lpddr3_odt",
+				    &timing->phy_lpddr3_odt);
+	ret |= of_property_read_u32(np, "lpddr4_odt_dis_freq",
+				    &timing->lpddr4_odt_dis_freq);
+	ret |= of_property_read_u32(np, "lpddr4_drv",
+				    &timing->lpddr4_drv);
+	ret |= of_property_read_u32(np, "lpddr4_dq_odt",
+				    &timing->lpddr4_dq_odt);
+	ret |= of_property_read_u32(np, "lpddr4_ca_odt",
+				    &timing->lpddr4_ca_odt);
+	ret |= of_property_read_u32(np, "phy_lpddr4_ca_drv",
+				    &timing->phy_lpddr4_ca_drv);
+	ret |= of_property_read_u32(np, "phy_lpddr4_ck_cs_drv",
+				    &timing->phy_lpddr4_ck_cs_drv);
+	ret |= of_property_read_u32(np, "phy_lpddr4_dq_drv",
+				    &timing->phy_lpddr4_dq_drv);
+	ret |= of_property_read_u32(np, "phy_lpddr4_odt",
+				    &timing->phy_lpddr4_odt);
+
+	return ret;
+}
+
+static struct dram_timing *of_get_ddr_timings(struct device *dev,
+					      struct device_node *np)
+{
+	struct dram_timing	*timing = NULL;
+	struct device_node	*np_tim;
+
+	np_tim = of_parse_phandle(np, "ddr_timing", 0);
+	if (np_tim) {
+		timing = devm_kzalloc(dev, sizeof(*timing), GFP_KERNEL);
+		if (!timing)
+			goto err;
+		if (of_do_get_timing(np_tim, timing)) {
+			devm_kfree(dev, timing);
+			goto err;
+		}
+		return timing;
+	}
+
+err:
+	if (timing) {
+		devm_kfree(dev, timing);
+		timing = NULL;
+	}
+
+	return timing;
+}
+
+static int rk3399_dmcfreq_probe(struct platform_device *pdev)
+{
+	struct arm_smccc_res res;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = pdev->dev.of_node;
+	uint64_t param = 0;
+	struct rk3399_dmcfreq *data;
+	int ret, irq, index;
+	uint32_t *timing;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(&pdev->dev, "no dmc irq resource\n");
+		return -EINVAL;
+	}
+	data = devm_kzalloc(dev, sizeof(struct rk3399_dmcfreq), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	mutex_init(&data->lock);
+
+	data->vdd_center = devm_regulator_get(dev, "center");
+	if (IS_ERR(data->vdd_center)) {
+		dev_err(dev, "Cannot get the regulator \"center\"\n");
+		return PTR_ERR(data->vdd_center);
+	}
+
+	data->dmc_clk = devm_clk_get(dev, "dmc_clk");
+	if (IS_ERR(data->dmc_clk)) {
+		dev_err(dev, "Cannot get the clk dmc_clk\n");
+		return PTR_ERR(data->dmc_clk);
+	};
+
+	data->irq = irq;
+	ret = devm_request_irq(dev, irq, rk3399_dmc_irq, 0,
+			       dev_name(dev), data);
+	if (ret) {
+		dev_err(dev, "failed to request dmc irq: %d\n", ret);
+		return ret;
+	}
+
+	/* get dram timing and pass it to bl31 */
+	data->timing = of_get_ddr_timings(dev, np);
+	if (data->timing) {
+		timing = (uint32_t *)data->timing;
+		for (index = 0; index < (sizeof(struct dram_timing) / 4);
+		     index++) {
+			param = index;
+			param = param << 32 | *timing++;
+			arm_smccc_smc(SIP_DDR_FREQ, param, 0,
+				      CONFIG_DRAM_SET_PARAM, 0, 0, 0, 0, &res);
+			if (res.a0) {
+				dev_err(dev, "failed to set dram param: %ld\n",
+					res.a0);
+				return -EINVAL;
+			}
+			param = 0;
+		}
+	}
+
+	arm_smccc_smc(SIP_DDR_FREQ, 0, 0, CONFIG_DRAM_INIT,
+		      0, 0, 0, 0, &res);
+
+	init_waitqueue_head(&data->wait_dcf_queue);
+	data->wait_dcf_flag = 0;
+
+	data->edev = devfreq_event_get_edev_by_phandle(dev, 0);
+	if (IS_ERR(data->edev))
+		return -EPROBE_DEFER;
+
+	ret = devfreq_event_enable_edev(data->edev);
+	if (ret < 0) {
+		dev_err(dev, "failed to enable devfreq-event devices\n");
+		return ret;
+	}
+
+	/*
+	 * We add a devfreq driver to our parent since it has a device tree node
+	 * with operating points.
+	 */
+	if (dev_pm_opp_of_add_table(dev)) {
+		dev_err(dev, "Invalid operating-points in device tree.\n");
+		return -EINVAL;
+	}
+	of_property_read_u32(np, "upthreshold",
+			     &data->ondemand_data.upthreshold);
+	of_property_read_u32(np, "downdifferential",
+			     &data->ondemand_data.downdifferential);
+	data->rate = clk_get_rate(data->dmc_clk);
+	rk3399_devfreq_dmc_profile.initial_freq = data->rate;
+	data->devfreq = devfreq_add_device(dev,
+					   &rk3399_devfreq_dmc_profile,
+					   "simple_ondemand",
+					   &data->ondemand_data);
+	if (IS_ERR(data->devfreq))
+		return PTR_ERR(data->devfreq);
+	devfreq_register_opp_notifier(dev, data->devfreq);
+
+	data->dev = dev;
+	platform_set_drvdata(pdev, data);
+
+	return 0;
+}
+
+static int rk3399_dmcfreq_remove(struct platform_device *pdev)
+{
+	struct rk3399_dmcfreq *dmcfreq = platform_get_drvdata(pdev);
+
+	devfreq_remove_device(dmcfreq->devfreq);
+	regulator_put(dmcfreq->vdd_center);
+
+	return 0;
+}
+
+static const struct of_device_id rk3399dmc_devfreq_of_match[] = {
+	{ .compatible = "rockchip,rk3399-dmc" },
+	{ },
+};
+
+static struct platform_driver rk3399_dmcfreq_driver = {
+	.probe	= rk3399_dmcfreq_probe,
+	.remove	= rk3399_dmcfreq_remove,
+	.driver = {
+		.name	= "rk3399-dmc-freq",
+		.pm	= &rk3399_dmcfreq_pm,
+		.of_match_table = rk3399dmc_devfreq_of_match,
+	},
+};
+module_platform_driver(rk3399_dmcfreq_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("RK3399 dmcfreq driver with devfreq framework");
-- 
1.9.1

Re: [PATCH v4 6/7] PM / devfreq: rockchip: add devfreq driver for rk3399 dmc

[Chanwoo Choi]

Hi Lin,

As I mentioned on patch5, you better to make the documentation as following: 
- Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
And, I add the comments.


On 2016년 07월 29일 16:57, Lin Huang wrote:
> base on dfi result, we do ddr frequency scaling, register
> dmc driver to devfreq framework, and use simple-ondemand
> policy.
> 
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> ---
> Changes in v4:
> - use arm_smccc_smc() function talk to bl31
> - delete rockchip_dmc.c file and config
> - delete dmc_notify
> - adjust probe order
>  
> Changes in v3:
> - operate dram setting through sip call
> - imporve set rate flow
> 
> Changes in v2:
> - None
>  
> Changes in v1:
> - move dfi controller to event
> - fix set voltage sequence when set rate fail
> - change Kconfig type from tristate to bool
> - move unuse EXPORT_SYMBOL_GPL()
> 
>  drivers/devfreq/Kconfig               |   1 +
>  drivers/devfreq/Makefile              |   1 +
>  drivers/devfreq/rockchip/Kconfig      |   8 +
>  drivers/devfreq/rockchip/Makefile     |   1 +
>  drivers/devfreq/rockchip/rk3399_dmc.c | 473 ++++++++++++++++++++++++++++++++++
>  5 files changed, 484 insertions(+)
>  create mode 100644 drivers/devfreq/rockchip/Kconfig
>  create mode 100644 drivers/devfreq/rockchip/Makefile
>  create mode 100644 drivers/devfreq/rockchip/rk3399_dmc.c
> 
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 64281bb..acb2a57 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -99,5 +99,6 @@ config ARM_TEGRA_DEVFREQ
>           operating frequencies and voltages with OPP support.
>  
>  source "drivers/devfreq/event/Kconfig"
> +source "drivers/devfreq/rockchip/Kconfig"

This patch include the only one patch. So, I think that
you don't need to create the 'rockchip' directory.

>  
>  endif # PM_DEVFREQ
> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> index 5134f9e..d844e23 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_DEVFREQ_GOV_USERSPACE)	+= governor_userspace.o
>  obj-$(CONFIG_ARM_EXYNOS4_BUS_DEVFREQ)	+= exynos/
>  obj-$(CONFIG_ARM_EXYNOS5_BUS_DEVFREQ)	+= exynos/
>  obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra-devfreq.o
> +obj-$(CONFIG_ARCH_ROCKCHIP)		+= rockchip/

ditto.

>  
>  # DEVFREQ Event Drivers
>  obj-$(CONFIG_PM_DEVFREQ_EVENT)		+= event/
> diff --git a/drivers/devfreq/rockchip/Kconfig b/drivers/devfreq/rockchip/Kconfig
> new file mode 100644
> index 0000000..d8f9e66
> --- /dev/null
> +++ b/drivers/devfreq/rockchip/Kconfig
> @@ -0,0 +1,8 @@
> +config ARM_RK3399_DMC_DEVFREQ
> +	tristate "ARM RK3399 DMC DEVFREQ Driver"
> +	select PM_OPP
> +	select DEVFREQ_GOV_SIMPLE_ONDEMAND
> +	help
> +          This adds the DEVFREQ driver for the RK3399 dmc. It sets the frequency

If you add the full description for 'dmc' as following,
it is easy to understand the operation of this device driver.
- DMC (Dynamic Memory Controller)

> +          for the memory controller and reads the usage counts from hardware.
> +
> diff --git a/drivers/devfreq/rockchip/Makefile b/drivers/devfreq/rockchip/Makefile
> new file mode 100644
> index 0000000..c62c105
> --- /dev/null
> +++ b/drivers/devfreq/rockchip/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
> diff --git a/drivers/devfreq/rockchip/rk3399_dmc.c b/drivers/devfreq/rockchip/rk3399_dmc.c
> new file mode 100644
> index 0000000..527aa11
> --- /dev/null
> +++ b/drivers/devfreq/rockchip/rk3399_dmc.c
> @@ -0,0 +1,473 @@
> +/*
> + * Copyright (c) 2016, Fuzhou Rockchip Electronics Co., Ltd

You miss the '.' at the end of the copylight. 
When you use an abbreviation, you should add '.' for Ltd.
- s/Ltd/Ltd.


> + * Author: Lin Huang <hl@rock-chips.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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/arm-smccc.h>
> +#include <linux/clk.h>
> +#include <linux/completion.h>

You don't need to include the "completion.h".
Without "completion.h", the build is working.

> +#include <linux/delay.h>
> +#include <linux/devfreq.h>
> +#include <linux/devfreq-event.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_opp.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/rwsem.h>
> +#include <linux/suspend.h>
> +#include <linux/syscore_ops.h>

You don't need to include the "syscore_ops.h".
Without "syscore_ops.h", the build is working.

> +
> +#include <soc/rockchip/rockchip_sip.h>
> +
> +struct dram_timing {
> +	unsigned int ddr3_speed_bin;
> +	unsigned int pd_idle;
> +	unsigned int sr_idle;
> +	unsigned int sr_mc_gate_idle;
> +	unsigned int srpd_lite_idle;
> +	unsigned int standby_idle;
> +	unsigned int dram_dll_dis_freq;
> +	unsigned int phy_dll_dis_freq;
> +	unsigned int ddr3_odt_dis_freq;
> +	unsigned int ddr3_drv;
> +	unsigned int ddr3_odt;
> +	unsigned int phy_ddr3_ca_drv;
> +	unsigned int phy_ddr3_dq_drv;
> +	unsigned int phy_ddr3_odt;
> +	unsigned int lpddr3_odt_dis_freq;
> +	unsigned int lpddr3_drv;
> +	unsigned int lpddr3_odt;
> +	unsigned int phy_lpddr3_ca_drv;
> +	unsigned int phy_lpddr3_dq_drv;
> +	unsigned int phy_lpddr3_odt;
> +	unsigned int lpddr4_odt_dis_freq;
> +	unsigned int lpddr4_drv;
> +	unsigned int lpddr4_dq_odt;
> +	unsigned int lpddr4_ca_odt;
> +	unsigned int phy_lpddr4_ca_drv;
> +	unsigned int phy_lpddr4_ck_cs_drv;
> +	unsigned int phy_lpddr4_dq_drv;
> +	unsigned int phy_lpddr4_odt;
> +};

The dram_timing is used for SMC (Secure Monitor Call)? 
I recommend that you add the detailed description about this property
on Documentation.

> +
> +struct rk3399_dmcfreq {
> +	struct device *dev;
> +	struct devfreq *devfreq;
> +	struct devfreq_simple_ondemand_data ondemand_data;
> +	struct clk *dmc_clk;
> +	struct devfreq_event_dev *edev;
> +	struct mutex lock;
> +	struct dram_timing *timing;
> +	wait_queue_head_t	wait_dcf_queue;
> +	int irq;
> +	int wait_dcf_flag;

I want to add the full name and description of 'dcf'.
It is unknown word.

> +	struct regulator *vdd_center;
> +	unsigned long rate, target_rate;
> +	unsigned long volt, target_volt;

I think that if you add 'curr_opp' variable,
you can reduce the calling of devfreq_recommended_opp() in rk3399_dmcfreq_target()
	struct dev_pm_opp *curr_opp;

> +};
> +
> +static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
> +				 u32 flags)
> +{
> +	struct platform_device *pdev = container_of(dev, struct platform_device,
> +						    dev);
> +	struct rk3399_dmcfreq *dmcfreq = platform_get_drvdata(pdev);

You can use the 'dev_get_drvdata()' to simplify it instead of 'platform_get_drvdata()'.

	struct rk3399_dmcfreq *dmcfreq = dev_get_drvdata(dev);

> +	struct dev_pm_opp *opp;
> +	unsigned long old_clk_rate = dmcfreq->rate;
> +	unsigned long target_volt, target_rate;
> +	int err;
> +
> +	rcu_read_lock();
> +	opp = devfreq_recommended_opp(dev, freq, flags);
> +	if (IS_ERR(opp)) {
> +		rcu_read_unlock();
> +		return PTR_ERR(opp);
> +	}
> +
> +	target_rate = dev_pm_opp_get_freq(opp);
> +	target_volt = dev_pm_opp_get_voltage(opp);
> +	opp = devfreq_recommended_opp(dev, &dmcfreq->rate, flags);
> +	if (IS_ERR(opp)) {
> +		rcu_read_unlock();
> +		return PTR_ERR(opp);
> +	}
> +	dmcfreq->volt = dev_pm_opp_get_voltage(opp);

If you add the 'curr_opp' variable to struct rk3399_dmcfreq,
you can remove the calling of devfreq_recommended_opp().
	dmcfreq->rate = dev_pm_opp_get_freq(dmcfreq->curr_opp);
	dmcfreq->volt = dev_pm_opp_get_freq(dmcfreq->curr_opp);

Because the current rate and voltage is already decided on previous polling cycle,
So we don't need to get the opp with devfreq_recommended_opp().

> +	rcu_read_unlock();
> +
> +	if (dmcfreq->rate == target_rate)
> +		return 0;
> +
> +	mutex_lock(&dmcfreq->lock);
> +
> +	/*
> +	 * if frequency scaling from low to high, adjust voltage first;
> +	 * if frequency scaling from high to low, adjuset frequency first;
> +	 */

s/adjuset/adjust

I recommend that you use a captital letter for first character and use the '.'
instead of ';'.

> +	if (old_clk_rate < target_rate) {
> +		err = regulator_set_voltage(dmcfreq->vdd_center, target_volt,
> +					    target_volt);
> +		if (err) {
> +			dev_err(dev, "Unable to set vol %lu\n", target_volt);

To readability, you better to use the corrent word to pass the precise the log message.
- s/vol/voltage

And, this patch uses the 'Unable to' or 'Cannot' to show the error log.
I recommend that you use the consistent expression if there is not any specific reason.

	dev_err(dev, "Cannot set the voltage %lu uV\n", target_volt);

> +			goto out;
> +		}
> +	}
> +	dmcfreq->wait_dcf_flag = 1;
> +	err = clk_set_rate(dmcfreq->dmc_clk, target_rate);
> +	if (err) {
> +		dev_err(dev,
> +			"Unable to set freq %lu. Current freq %lu. Error %d\n",
> +			target_rate, old_clk_rate, err);

	dev_err(dev, "Cannot set the frequency %lu (%d)\n", target_rate, err);

> +		regulator_set_voltage(dmcfreq->vdd_center, dmcfreq->volt,
> +				      dmcfreq->volt);
> +		goto out;
> +	}
> +
> +	/*
> +	 * wait until bcf irq happen, it means freq scaling finish in bl31,

ditto.

> +	 * use 100ms as timeout time

s/time/time.

> +	 */
> +	if (!wait_event_timeout(dmcfreq->wait_dcf_queue,
> +				!dmcfreq->wait_dcf_flag, HZ / 10))
> +		dev_warn(dev, "Timeout waiting for dcf irq\n");

If the timeout happen, are there any problem?

After setting the frequency and voltage, store the current opp entry on .curr_opp.
	dmcfreq->curr_opp = opp;

> +	/*
> +	 * check the dpll rate
> +	 * there only two result we will get,
> +	 * 1. ddr frequency scaling fail, we still get the old rate
> +	 * 2, ddr frequency scaling sucessful, we get the rate we set
> +	 */
> +	dmcfreq->rate = clk_get_rate(dmcfreq->dmc_clk);
> +
> +	/* if get the incorrect rate, set voltage to old value */
> +	if (dmcfreq->rate != target_rate) {
> +		dev_err(dev, "get wrong ddr frequency, Request freq %lu,\
> +			Current freq %lu\n", target_rate, dmcfreq->rate);
> +		regulator_set_voltage(dmcfreq->vdd_center, dmcfreq->volt,
> +				      dmcfreq->volt);

[Without force, it is just my opion about this code:]
I think that this checking code it is un-needed.
If this case occur, the rk3399_dmc.c never set the specific frequency
because the rk3399 clock don't support the specific frequency for rk3399 dmc.

If you want to set the correct frequency,
When verifying the rk3399 dmc driver, you should check the rk3399 clock driver.

Basically, if the the clock driver don't support the correct same frequency ,
CCF(Common Clock Framework) set the close frequency. It is not a bad thing.


> +	} else if (old_clk_rate > target_rate)
> +		err = regulator_set_voltage(dmcfreq->vdd_center, target_volt,
> +					    target_volt);
> +	if (err)
> +		dev_err(dev, "Unable to set vol %lu\n", target_volt);
	
ditto. You better to use the consistent expression for error log as following:
	dev_err(dev, "Cannot set the voltage %lu uV\n", target_volt);


> +
> +out:
> +	mutex_unlock(&dmcfreq->lock);
> +	return err;
> +}
> +
> +static int rk3399_dmcfreq_get_dev_status(struct device *dev,
> +					 struct devfreq_dev_status *stat)
> +{
> +	struct platform_device *pdev = container_of(dev, struct platform_device,
> +						    dev);
> +	struct rk3399_dmcfreq *dmcfreq = platform_get_drvdata(pdev);
> +	struct devfreq_event_data edata;

ditto. Use the dev_get_drvdata(dev).

> +
> +	devfreq_event_get_event(dmcfreq->edev, &edata);

You need to check the return value for exception handling.

> +
> +	stat->current_frequency = dmcfreq->rate;
> +	stat->busy_time = edata.load_count;
> +	stat->total_time = edata.total_count;
> +
> +	return 0;
> +}
> +
> +static int rk3399_dmcfreq_get_cur_freq(struct device *dev, unsigned long *freq)
> +{
> +	struct platform_device *pdev = container_of(dev, struct platform_device,
> +						    dev);
> +	struct rk3399_dmcfreq *dmcfreq = platform_get_drvdata(pdev);

ditto. Use the dev_get_drvdata(dev).

> +
> +	*freq = dmcfreq->rate;
> +
> +	return 0;
> +}
> +
> +static void rk3399_dmcfreq_exit(struct device *dev)
> +{
> +	struct platform_device *pdev = container_of(dev,
> +						    struct platform_device,
> +						    dev);
> +	struct rk3399_dmcfreq *dmcfreq = platform_get_drvdata(pdev);
> +
> +	devfreq_unregister_opp_notifier(dev, dmcfreq->devfreq);

Use the devm_devfreq_register_opp_notifier(). You don't need to handle it.

> +}
> +
> +static struct devfreq_dev_profile rk3399_devfreq_dmc_profile = {
> +	.polling_ms	= 200,
> +	.target		= rk3399_dmcfreq_target,
> +	.get_dev_status	= rk3399_dmcfreq_get_dev_status,
> +	.get_cur_freq	= rk3399_dmcfreq_get_cur_freq,
> +	.exit		= rk3399_dmcfreq_exit,
> +};
> +
> +static __maybe_unused int rk3399_dmcfreq_suspend(struct device *dev)
> +{
> +	struct platform_device *pdev = container_of(dev,
> +						    struct platform_device,
> +						    dev);
> +	struct rk3399_dmcfreq *dmcfreq = platform_get_drvdata(pdev);

ditto. Use the dev_get_drvdata(dev).

> +
> +	devfreq_event_disable_edev(dmcfreq->edev);
> +	devfreq_suspend_device(dmcfreq->devfreq);

ditto. you need to check the return value.

> +
> +	return 0;
> +}
> +
> +static __maybe_unused int rk3399_dmcfreq_resume(struct device *dev)
> +{
> +	struct platform_device *pdev = container_of(dev,
> +						    struct platform_device,
> +						    dev);
> +	struct rk3399_dmcfreq *dmcfreq = platform_get_drvdata(pdev);

ditto.

> +
> +	devfreq_event_enable_edev(dmcfreq->edev);
> +	devfreq_resume_device(dmcfreq->devfreq);

ditto.

> +
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(rk3399_dmcfreq_pm, rk3399_dmcfreq_suspend,
> +			 rk3399_dmcfreq_resume);
> +
> +static irqreturn_t rk3399_dmc_irq(int irq, void *dev_id)
> +{
> +	struct rk3399_dmcfreq *dmcfreq = dev_id;
> +	struct arm_smccc_res res;
> +
> +	dmcfreq->wait_dcf_flag = 0;
> +	wake_up(&dmcfreq->wait_dcf_queue);
> +
> +	/* clr dcf irq */

s/ clr dcf irq -> "Clear the DCF Interrupt"

> +	arm_smccc_smc(SIP_DDR_FREQ, 0, 0, CONFIG_DRAM_CLR_IRQ,
> +		      0, 0, 0, 0, &res);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int of_do_get_timing(struct device_node *np,
> +		struct dram_timing *timing)
> +{
> +	int ret;
> +
> +	ret = of_property_read_u32(np, "ddr3_speed_bin",
> +				   &timing->ddr3_speed_bin);
> +	ret |= of_property_read_u32(np, "pd_idle", &timing->pd_idle);
> +	ret |= of_property_read_u32(np, "sr_idle", &timing->sr_idle);
> +	ret |= of_property_read_u32(np, "sr_mc_gate_idle",
> +				    &timing->sr_mc_gate_idle);
> +	ret |= of_property_read_u32(np, "srpd_lite_idle",
> +				    &timing->srpd_lite_idle);
> +	ret |= of_property_read_u32(np, "standby_idle", &timing->standby_idle);
> +	ret |= of_property_read_u32(np, "dram_dll_dis_freq",
> +				    &timing->dram_dll_dis_freq);
> +	ret |= of_property_read_u32(np, "phy_dll_dis_freq",
> +				    &timing->phy_dll_dis_freq);
> +	ret |= of_property_read_u32(np, "ddr3_odt_dis_freq",
> +				    &timing->ddr3_odt_dis_freq);
> +	ret |= of_property_read_u32(np, "ddr3_drv", &timing->ddr3_drv);
> +	ret |= of_property_read_u32(np, "ddr3_odt", &timing->ddr3_odt);
> +	ret |= of_property_read_u32(np, "phy_ddr3_ca_drv",
> +				    &timing->phy_ddr3_ca_drv);
> +	ret |= of_property_read_u32(np, "phy_ddr3_dq_drv",
> +				    &timing->phy_ddr3_dq_drv);
> +	ret |= of_property_read_u32(np, "phy_ddr3_odt", &timing->phy_ddr3_odt);
> +	ret |= of_property_read_u32(np, "lpddr3_odt_dis_freq",
> +				    &timing->lpddr3_odt_dis_freq);
> +	ret |= of_property_read_u32(np, "lpddr3_drv", &timing->lpddr3_drv);
> +	ret |= of_property_read_u32(np, "lpddr3_odt", &timing->lpddr3_odt);
> +	ret |= of_property_read_u32(np, "phy_lpddr3_ca_drv",
> +				    &timing->phy_lpddr3_ca_drv);
> +	ret |= of_property_read_u32(np, "phy_lpddr3_dq_drv",
> +				    &timing->phy_lpddr3_dq_drv);
> +	ret |= of_property_read_u32(np, "phy_lpddr3_odt",
> +				    &timing->phy_lpddr3_odt);
> +	ret |= of_property_read_u32(np, "lpddr4_odt_dis_freq",
> +				    &timing->lpddr4_odt_dis_freq);
> +	ret |= of_property_read_u32(np, "lpddr4_drv",
> +				    &timing->lpddr4_drv);
> +	ret |= of_property_read_u32(np, "lpddr4_dq_odt",
> +				    &timing->lpddr4_dq_odt);
> +	ret |= of_property_read_u32(np, "lpddr4_ca_odt",
> +				    &timing->lpddr4_ca_odt);
> +	ret |= of_property_read_u32(np, "phy_lpddr4_ca_drv",
> +				    &timing->phy_lpddr4_ca_drv);
> +	ret |= of_property_read_u32(np, "phy_lpddr4_ck_cs_drv",
> +				    &timing->phy_lpddr4_ck_cs_drv);
> +	ret |= of_property_read_u32(np, "phy_lpddr4_dq_drv",
> +				    &timing->phy_lpddr4_dq_drv);
> +	ret |= of_property_read_u32(np, "phy_lpddr4_odt",
> +				    &timing->phy_lpddr4_odt);
> +
> +	return ret;
> +}
> +
> +static struct dram_timing *of_get_ddr_timings(struct device *dev,
> +					      struct device_node *np)
> +{
> +	struct dram_timing	*timing = NULL;
> +	struct device_node	*np_tim;
> +
> +	np_tim = of_parse_phandle(np, "ddr_timing", 0);

As I already mentioned, you need to add the detailed documetation about the property.

> +	if (np_tim) {
> +		timing = devm_kzalloc(dev, sizeof(*timing), GFP_KERNEL);
> +		if (!timing)
> +			goto err;
> +		if (of_do_get_timing(np_tim, timing)) {

I recommend that you better to squash following two functions
because of_do_get_timing() is not used on other point of this driver.
- of_do_get_timing()
- of_get_ddr_timings()

> +			devm_kfree(dev, timing);
> +			goto err;
> +		}

You're missing the 'of_node_put'.
		of_node_put(np_tim);

> +		return timing;
> +	}
> +
> +err:
> +	if (timing) {
> +		devm_kfree(dev, timing);
> +		timing = NULL;
> +	}
> +

ditto.
	of_node_put(np_tim);

> +	return timing;
> +}
> +
> +static int rk3399_dmcfreq_probe(struct platform_device *pdev)
> +{
> +	struct arm_smccc_res res;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = pdev->dev.of_node;
> +	uint64_t param = 0;
> +	struct rk3399_dmcfreq *data;
> +	int ret, irq, index;
> +	uint32_t *timing;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		dev_err(&pdev->dev, "no dmc irq resource\n");

We need to maintain the consistent expression for error log.
		dev_err(&pdev->dev, ""Cannot get the interrupt resource"\n");
		
> +		return -EINVAL;
> +	}
> +	data = devm_kzalloc(dev, sizeof(struct rk3399_dmcfreq), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	mutex_init(&data->lock);
> +
> +	data->vdd_center = devm_regulator_get(dev, "center");
> +	if (IS_ERR(data->vdd_center)) {
> +		dev_err(dev, "Cannot get the regulator \"center\"\n");
> +		return PTR_ERR(data->vdd_center);
> +	}
> +
> +	data->dmc_clk = devm_clk_get(dev, "dmc_clk");
> +	if (IS_ERR(data->dmc_clk)) {
> +		dev_err(dev, "Cannot get the clk dmc_clk\n");
> +		return PTR_ERR(data->dmc_clk);
> +	};
> +
> +	data->irq = irq;
> +	ret = devm_request_irq(dev, irq, rk3399_dmc_irq, 0,
> +			       dev_name(dev), data);
> +	if (ret) {
> +		dev_err(dev, "failed to request dmc irq: %d\n", ret);

ditto. "Failed to request the dmc irq" or "Cannot request the dmc irq"

> +		return ret;
> +	}
> +
> +	/* get dram timing and pass it to bl31 */

I want to add the more detailed description why this code is necessary.
Because the SMC is usually black box. So, If you add the more explanation,
it help people to understand this code. 

> +	data->timing = of_get_ddr_timings(dev, np);
> +	if (data->timing) {
> +		timing = (uint32_t *)data->timing;
> +		for (index = 0; index < (sizeof(struct dram_timing) / 4);
> +		     index++) {

You better to use following code. It is better way to use
the only one line for 'for' statement.

		size = sizeof(struct dram_timing) / 4;
		for (index = 0; index < size; index++) {

> +			param = index;
> +			param = param << 32 | *timing++;
> +			arm_smccc_smc(SIP_DDR_FREQ, param, 0,
> +				      CONFIG_DRAM_SET_PARAM, 0, 0, 0, 0, &res);
> +			if (res.a0) {
> +				dev_err(dev, "failed to set dram param: %ld\n",
> +					res.a0);
> +				return -EINVAL;
> +			}
> +			param = 0;
> +		}
> +	}
> +
> +	arm_smccc_smc(SIP_DDR_FREQ, 0, 0, CONFIG_DRAM_INIT,
> +		      0, 0, 0, 0, &res);

ditto. You need to add the description.

> +
> +	init_waitqueue_head(&data->wait_dcf_queue);
> +	data->wait_dcf_flag = 0;
> +
> +	data->edev = devfreq_event_get_edev_by_phandle(dev, 0);
> +	if (IS_ERR(data->edev))
> +		return -EPROBE_DEFER;
> +
> +	ret = devfreq_event_enable_edev(data->edev);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to enable devfreq-event devices\n");

s/failed/Failed. Use a capital letter for the first char.

> +		return ret;
> +	}
> +
> +	/*
> +	 * We add a devfreq driver to our parent since it has a device tree node
> +	 * with operating points.
> +	 */

You should wrap the rcu lock before calling the dev_pm_opp_of_add_table().

> +	if (dev_pm_opp_of_add_table(dev)) {
> +		dev_err(dev, "Invalid operating-points in device tree.\n");
> +		return -EINVAL;
> +	}
> +	of_property_read_u32(np, "upthreshold",
> +			     &data->ondemand_data.upthreshold);
> +	of_property_read_u32(np, "downdifferential",
> +			     &data->ondemand_data.downdifferential);

Need one blank line.
Maybe this code to get the properfy for ondemand governor,
the devfreq will support in the future.

> +	data->rate = clk_get_rate(data->dmc_clk);
> +	rk3399_devfreq_dmc_profile.initial_freq = data->rate;

Need one blank line.

> +	data->devfreq = devfreq_add_device(dev,
> +					   &rk3399_devfreq_dmc_profile,
> +					   "simple_ondemand",
> +					   &data->ondemand_data);
> +	if (IS_ERR(data->devfreq))
> +		return PTR_ERR(data->devfreq);
> +	devfreq_register_opp_notifier(dev, data->devfreq);

Use the devm_devfreq_register_opp_notifier().

> +
> +	data->dev = dev;
> +	platform_set_drvdata(pdev, data);
> +
> +	return 0;
> +}
> +
> +static int rk3399_dmcfreq_remove(struct platform_device *pdev)
> +{
> +	struct rk3399_dmcfreq *dmcfreq = platform_get_drvdata(pdev);
> +
> +	devfreq_remove_device(dmcfreq->devfreq);
> +	regulator_put(dmcfreq->vdd_center);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id rk3399dmc_devfreq_of_match[] = {
> +	{ .compatible = "rockchip,rk3399-dmc" },
> +	{ },
> +};
> +
> +static struct platform_driver rk3399_dmcfreq_driver = {
> +	.probe	= rk3399_dmcfreq_probe,
> +	.remove	= rk3399_dmcfreq_remove,
> +	.driver = {
> +		.name	= "rk3399-dmc-freq",
> +		.pm	= &rk3399_dmcfreq_pm,
> +		.of_match_table = rk3399dmc_devfreq_of_match,
> +	},
> +};
> +module_platform_driver(rk3399_dmcfreq_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("RK3399 dmcfreq driver with devfreq framework");

You need to add the MODULE_AUTHOR information.

Regards,
Chanwoo Choi

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 6/7] PM / devfreq: rockchip: add devfreq driver for rk3399 dmc

[hl]

Hi Chanwoo Choi,

     Thanks for reviewing so carefully. And i have some question:

On 2016年08月01日 18:28, Chanwoo Choi wrote:
> Hi Lin,
>
> As I mentioned on patch5, you better to make the documentation as following:
> - Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
> And, I add the comments.
>
>
> On 2016년 07월 29일 16:57, Lin Huang wrote:
>> base on dfi result, we do ddr frequency scaling, register
>> dmc driver to devfreq framework, and use simple-ondemand
>> policy.
>>
>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>> ---
>> Changes in v4:
>> - use arm_smccc_smc() function talk to bl31
>> - delete rockchip_dmc.c file and config
>> - delete dmc_notify
>> - adjust probe order
>>   
>> Changes in v3:
>> - operate dram setting through sip call
>> - imporve set rate flow
>>
>> Changes in v2:
>> - None
>>   
>> Changes in v1:
>> - move dfi controller to event
>> - fix set voltage sequence when set rate fail
>> - change Kconfig type from tristate to bool
>> - move unuse EXPORT_SYMBOL_GPL()
>>
>>   drivers/devfreq/Kconfig               |   1 +
>>   drivers/devfreq/Makefile              |   1 +
>>   drivers/devfreq/rockchip/Kconfig      |   8 +
>>   drivers/devfreq/rockchip/Makefile     |   1 +
>>   drivers/devfreq/rockchip/rk3399_dmc.c | 473 ++++++++++++++++++++++++++++++++++
>>   5 files changed, 484 insertions(+)
>>   create mode 100644 drivers/devfreq/rockchip/Kconfig
>>   create mode 100644 drivers/devfreq/rockchip/Makefile
>>   create mode 100644 drivers/devfreq/rockchip/rk3399_dmc.c
>>
>> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>> index 64281bb..acb2a57 100644
>> --- a/drivers/devfreq/Kconfig
>> +++ b/drivers/devfreq/Kconfig
>> @@ -99,5 +99,6 @@ config ARM_TEGRA_DEVFREQ
>>            operating frequencies and voltages with OPP support.
>>   
>>   source "drivers/devfreq/event/Kconfig"
>> +source "drivers/devfreq/rockchip/Kconfig"
> This patch include the only one patch. So, I think that
> you don't need to create the 'rockchip' directory.
>
>>   
>>   endif # PM_DEVFREQ
>> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
>> index 5134f9e..d844e23 100644
>> --- a/drivers/devfreq/Makefile
>> +++ b/drivers/devfreq/Makefile
>> @@ -9,6 +9,7 @@ obj-$(CONFIG_DEVFREQ_GOV_USERSPACE)	+= governor_userspace.o
>>   obj-$(CONFIG_ARM_EXYNOS4_BUS_DEVFREQ)	+= exynos/
>>   obj-$(CONFIG_ARM_EXYNOS5_BUS_DEVFREQ)	+= exynos/
>>   obj-$(CONFIG_ARM_TEGRA_DEVFREQ)		+= tegra-devfreq.o
>> +obj-$(CONFIG_ARCH_ROCKCHIP)		+= rockchip/
> ditto.
>
>>   
>>   # DEVFREQ Event Drivers
>>   obj-$(CONFIG_PM_DEVFREQ_EVENT)		+= event/
>> diff --git a/drivers/devfreq/rockchip/Kconfig b/drivers/devfreq/rockchip/Kconfig
>> new file mode 100644
>> index 0000000..d8f9e66
>> --- /dev/null
>> +++ b/drivers/devfreq/rockchip/Kconfig
>> @@ -0,0 +1,8 @@
>> +config ARM_RK3399_DMC_DEVFREQ
>> +	tristate "ARM RK3399 DMC DEVFREQ Driver"
>> +	select PM_OPP
>> +	select DEVFREQ_GOV_SIMPLE_ONDEMAND
>> +	help
>> +          This adds the DEVFREQ driver for the RK3399 dmc. It sets the frequency
> If you add the full description for 'dmc' as following,
> it is easy to understand the operation of this device driver.
> - DMC (Dynamic Memory Controller)
>
>> +          for the memory controller and reads the usage counts from hardware.
>> +
>> diff --git a/drivers/devfreq/rockchip/Makefile b/drivers/devfreq/rockchip/Makefile
>> new file mode 100644
>> index 0000000..c62c105
>> --- /dev/null
>> +++ b/drivers/devfreq/rockchip/Makefile
>> @@ -0,0 +1 @@
>> +obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ)	+= rk3399_dmc.o
>> diff --git a/drivers/devfreq/rockchip/rk3399_dmc.c b/drivers/devfreq/rockchip/rk3399_dmc.c
>> new file mode 100644
>> index 0000000..527aa11
>> --- /dev/null
>> +++ b/drivers/devfreq/rockchip/rk3399_dmc.c
>> @@ -0,0 +1,473 @@
>> +/*
>> + * Copyright (c) 2016, Fuzhou Rockchip Electronics Co., Ltd
> You miss the '.' at the end of the copylight.
> When you use an abbreviation, you should add '.' for Ltd.
> - s/Ltd/Ltd.
>
>
>> + * Author: Lin Huang <hl@rock-chips.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope 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/arm-smccc.h>
>> +#include <linux/clk.h>
>> +#include <linux/completion.h>
> You don't need to include the "completion.h".
> Without "completion.h", the build is working.
>
>> +#include <linux/delay.h>
>> +#include <linux/devfreq.h>
>> +#include <linux/devfreq-event.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_opp.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/rwsem.h>
>> +#include <linux/suspend.h>
>> +#include <linux/syscore_ops.h>
> You don't need to include the "syscore_ops.h".
> Without "syscore_ops.h", the build is working.
>
>> +
>> +#include <soc/rockchip/rockchip_sip.h>
>> +
>> +struct dram_timing {
>> +	unsigned int ddr3_speed_bin;
>> +	unsigned int pd_idle;
>> +	unsigned int sr_idle;
>> +	unsigned int sr_mc_gate_idle;
>> +	unsigned int srpd_lite_idle;
>> +	unsigned int standby_idle;
>> +	unsigned int dram_dll_dis_freq;
>> +	unsigned int phy_dll_dis_freq;
>> +	unsigned int ddr3_odt_dis_freq;
>> +	unsigned int ddr3_drv;
>> +	unsigned int ddr3_odt;
>> +	unsigned int phy_ddr3_ca_drv;
>> +	unsigned int phy_ddr3_dq_drv;
>> +	unsigned int phy_ddr3_odt;
>> +	unsigned int lpddr3_odt_dis_freq;
>> +	unsigned int lpddr3_drv;
>> +	unsigned int lpddr3_odt;
>> +	unsigned int phy_lpddr3_ca_drv;
>> +	unsigned int phy_lpddr3_dq_drv;
>> +	unsigned int phy_lpddr3_odt;
>> +	unsigned int lpddr4_odt_dis_freq;
>> +	unsigned int lpddr4_drv;
>> +	unsigned int lpddr4_dq_odt;
>> +	unsigned int lpddr4_ca_odt;
>> +	unsigned int phy_lpddr4_ca_drv;
>> +	unsigned int phy_lpddr4_ck_cs_drv;
>> +	unsigned int phy_lpddr4_dq_drv;
>> +	unsigned int phy_lpddr4_odt;
>> +};
> The dram_timing is used for SMC (Secure Monitor Call)?
> I recommend that you add the detailed description about this property
> on Documentation.
>
>> +
>> +struct rk3399_dmcfreq {
>> +	struct device *dev;
>> +	struct devfreq *devfreq;
>> +	struct devfreq_simple_ondemand_data ondemand_data;
>> +	struct clk *dmc_clk;
>> +	struct devfreq_event_dev *edev;
>> +	struct mutex lock;
>> +	struct dram_timing *timing;
>> +	wait_queue_head_t	wait_dcf_queue;
>> +	int irq;
>> +	int wait_dcf_flag;
> I want to add the full name and description of 'dcf'.
> It is unknown word.
>
>> +	struct regulator *vdd_center;
>> +	unsigned long rate, target_rate;
>> +	unsigned long volt, target_volt;
> I think that if you add 'curr_opp' variable,
> you can reduce the calling of devfreq_recommended_opp() in rk3399_dmcfreq_target()
> 	struct dev_pm_opp *curr_opp;
>
>> +};
>> +
>> +static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
>> +				 u32 flags)
>> +{
>> +	struct platform_device *pdev = container_of(dev, struct platform_device,
>> +						    dev);
>> +	struct rk3399_dmcfreq *dmcfreq = platform_get_drvdata(pdev);
> You can use the 'dev_get_drvdata()' to simplify it instead of 'platform_get_drvdata()'.
>
> 	struct rk3399_dmcfreq *dmcfreq = dev_get_drvdata(dev);
>
>> +	struct dev_pm_opp *opp;
>> +	unsigned long old_clk_rate = dmcfreq->rate;
>> +	unsigned long target_volt, target_rate;
>> +	int err;
>> +
>> +	rcu_read_lock();
>> +	opp = devfreq_recommended_opp(dev, freq, flags);
>> +	if (IS_ERR(opp)) {
>> +		rcu_read_unlock();
>> +		return PTR_ERR(opp);
>> +	}
>> +
>> +	target_rate = dev_pm_opp_get_freq(opp);
>> +	target_volt = dev_pm_opp_get_voltage(opp);
>> +	opp = devfreq_recommended_opp(dev, &dmcfreq->rate, flags);
>> +	if (IS_ERR(opp)) {
>> +		rcu_read_unlock();
>> +		return PTR_ERR(opp);
>> +	}
>> +	dmcfreq->volt = dev_pm_opp_get_voltage(opp);
> If you add the 'curr_opp' variable to struct rk3399_dmcfreq,
> you can remove the calling of devfreq_recommended_opp().
> 	dmcfreq->rate = dev_pm_opp_get_freq(dmcfreq->curr_opp);
> 	dmcfreq->volt = dev_pm_opp_get_freq(dmcfreq->curr_opp);
>
> Because the current rate and voltage is already decided on previous polling cycle,
> So we don't need to get the opp with devfreq_recommended_opp().
I prefer the way now use, since we get the dmcfreq->rate use 
clk_get_rate() after,
Base on that,  i do not care the set_rate success or fail. use curr_opp 
i need to
care about set_rate status, when fail, i must set some rate, when 
success i must
set other rate.
>> +	rcu_read_unlock();
>> +
>> +	if (dmcfreq->rate == target_rate)
>> +		return 0;
>> +
>> +	mutex_lock(&dmcfreq->lock);
>> +
>> +	/*
>> +	 * if frequency scaling from low to high, adjust voltage first;
>> +	 * if frequency scaling from high to low, adjuset frequency first;
>> +	 */
> s/adjuset/adjust
>
> I recommend that you use a captital letter for first character and use the '.'
> instead of ';'.
>
>> +	if (old_clk_rate < target_rate) {
>> +		err = regulator_set_voltage(dmcfreq->vdd_center, target_volt,
>> +					    target_volt);
>> +		if (err) {
>> +			dev_err(dev, "Unable to set vol %lu\n", target_volt);
> To readability, you better to use the corrent word to pass the precise the log message.
> - s/vol/voltage
>
> And, this patch uses the 'Unable to' or 'Cannot' to show the error log.
> I recommend that you use the consistent expression if there is not any specific reason.
>
> 	dev_err(dev, "Cannot set the voltage %lu uV\n", target_volt);
>
>> +			goto out;
>> +		}
>> +	}
>> +	dmcfreq->wait_dcf_flag = 1;
>> +	err = clk_set_rate(dmcfreq->dmc_clk, target_rate);
>> +	if (err) {
>> +		dev_err(dev,
>> +			"Unable to set freq %lu. Current freq %lu. Error %d\n",
>> +			target_rate, old_clk_rate, err);
> 	dev_err(dev, "Cannot set the frequency %lu (%d)\n", target_rate, err);
>
>> +		regulator_set_voltage(dmcfreq->vdd_center, dmcfreq->volt,
>> +				      dmcfreq->volt);
>> +		goto out;
>> +	}
>> +
>> +	/*
>> +	 * wait until bcf irq happen, it means freq scaling finish in bl31,
> ditto.
>
>> +	 * use 100ms as timeout time
> s/time/time.
>
>> +	 */
>> +	if (!wait_event_timeout(dmcfreq->wait_dcf_queue,
>> +				!dmcfreq->wait_dcf_flag, HZ / 10))
>> +		dev_warn(dev, "Timeout waiting for dcf irq\n");
> If the timeout happen, are there any problem?
When timeout happen , may be we miss interrupt, but it do not affect this
process, since we will check the rate whether success later.
> After setting the frequency and voltage, store the current opp entry on .curr_opp.
> 	dmcfreq->curr_opp = opp;
>
>> +	/*
>> +	 * check the dpll rate
>> +	 * there only two result we will get,
>> +	 * 1. ddr frequency scaling fail, we still get the old rate
>> +	 * 2, ddr frequency scaling sucessful, we get the rate we set
>> +	 */
>> +	dmcfreq->rate = clk_get_rate(dmcfreq->dmc_clk);
>> +
>> +	/* if get the incorrect rate, set voltage to old value */
>> +	if (dmcfreq->rate != target_rate) {
>> +		dev_err(dev, "get wrong ddr frequency, Request freq %lu,\
>> +			Current freq %lu\n", target_rate, dmcfreq->rate);
>> +		regulator_set_voltage(dmcfreq->vdd_center, dmcfreq->volt,
>> +				      dmcfreq->volt);
> [Without force, it is just my opion about this code:]
> I think that this checking code it is un-needed.
> If this case occur, the rk3399_dmc.c never set the specific frequency
> because the rk3399 clock don't support the specific frequency for rk3399 dmc.
>
> If you want to set the correct frequency,
> When verifying the rk3399 dmc driver, you should check the rk3399 clock driver.
>
> Basically, if the the clock driver don't support the correct same frequency ,
> CCF(Common Clock Framework) set the close frequency. It is not a bad thing.
May be i should remove the regulator_set_voltage() here, but still need to
check whether we get the right frequency, since if we do not get the 
right frequency,
we should send a warn message, to remind that maybe you pass a frequency 
which
do not support in bl31.
>
>
>> +	} else if (old_clk_rate > target_rate)
>> +		err = regulator_set_voltage(dmcfreq->vdd_center, target_volt,
>> +					    target_volt);
>> +	if (err)
>> +		dev_err(dev, "Unable to set vol %lu\n", target_volt);
> 	
> ditto. You better to use the consistent expression for error log as following:
> 	dev_err(dev, "Cannot set the voltage %lu uV\n", target_volt);
>
>
>> +
>> +out:
>> +	mutex_unlock(&dmcfreq->lock);
>> +	return err;
>> +}
>> +
>> +static int rk3399_dmcfreq_get_dev_status(struct device *dev,
>> +					 struct devfreq_dev_status *stat)
>> +{
>> +	struct platform_device *pdev = container_of(dev, struct platform_device,
>> +						    dev);
>> +	struct rk3399_dmcfreq *dmcfreq = platform_get_drvdata(pdev);
>> +	struct devfreq_event_data edata;
> ditto. Use the dev_get_drvdata(dev).
>
>> +
>> +	devfreq_event_get_event(dmcfreq->edev, &edata);
> You need to check the return value for exception handling.
>
>> +
>> +	stat->current_frequency = dmcfreq->rate;
>> +	stat->busy_time = edata.load_count;
>> +	stat->total_time = edata.total_count;
>> +
>> +	return 0;
>> +}
>> +
>> +static int rk3399_dmcfreq_get_cur_freq(struct device *dev, unsigned long *freq)
>> +{
>> +	struct platform_device *pdev = container_of(dev, struct platform_device,
>> +						    dev);
>> +	struct rk3399_dmcfreq *dmcfreq = platform_get_drvdata(pdev);
> ditto. Use the dev_get_drvdata(dev).
>
>> +
>> +	*freq = dmcfreq->rate;
>> +
>> +	return 0;
>> +}
>> +
>> +static void rk3399_dmcfreq_exit(struct device *dev)
>> +{
>> +	struct platform_device *pdev = container_of(dev,
>> +						    struct platform_device,
>> +						    dev);
>> +	struct rk3399_dmcfreq *dmcfreq = platform_get_drvdata(pdev);
>> +
>> +	devfreq_unregister_opp_notifier(dev, dmcfreq->devfreq);
> Use the devm_devfreq_register_opp_notifier(). You don't need to handle it.
>
>> +}
>> +
>> +static struct devfreq_dev_profile rk3399_devfreq_dmc_profile = {
>> +	.polling_ms	= 200,
>> +	.target		= rk3399_dmcfreq_target,
>> +	.get_dev_status	= rk3399_dmcfreq_get_dev_status,
>> +	.get_cur_freq	= rk3399_dmcfreq_get_cur_freq,
>> +	.exit		= rk3399_dmcfreq_exit,
>> +};
>> +
>> +static __maybe_unused int rk3399_dmcfreq_suspend(struct device *dev)
>> +{
>> +	struct platform_device *pdev = container_of(dev,
>> +						    struct platform_device,
>> +						    dev);
>> +	struct rk3399_dmcfreq *dmcfreq = platform_get_drvdata(pdev);
> ditto. Use the dev_get_drvdata(dev).
>
>> +
>> +	devfreq_event_disable_edev(dmcfreq->edev);
>> +	devfreq_suspend_device(dmcfreq->devfreq);
> ditto. you need to check the return value.
>
>> +
>> +	return 0;
>> +}
>> +
>> +static __maybe_unused int rk3399_dmcfreq_resume(struct device *dev)
>> +{
>> +	struct platform_device *pdev = container_of(dev,
>> +						    struct platform_device,
>> +						    dev);
>> +	struct rk3399_dmcfreq *dmcfreq = platform_get_drvdata(pdev);
> ditto.
>
>> +
>> +	devfreq_event_enable_edev(dmcfreq->edev);
>> +	devfreq_resume_device(dmcfreq->devfreq);
> ditto.
>
>> +
>> +	return 0;
>> +}
>> +
>> +static SIMPLE_DEV_PM_OPS(rk3399_dmcfreq_pm, rk3399_dmcfreq_suspend,
>> +			 rk3399_dmcfreq_resume);
>> +
>> +static irqreturn_t rk3399_dmc_irq(int irq, void *dev_id)
>> +{
>> +	struct rk3399_dmcfreq *dmcfreq = dev_id;
>> +	struct arm_smccc_res res;
>> +
>> +	dmcfreq->wait_dcf_flag = 0;
>> +	wake_up(&dmcfreq->wait_dcf_queue);
>> +
>> +	/* clr dcf irq */
> s/ clr dcf irq -> "Clear the DCF Interrupt"
>
>> +	arm_smccc_smc(SIP_DDR_FREQ, 0, 0, CONFIG_DRAM_CLR_IRQ,
>> +		      0, 0, 0, 0, &res);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int of_do_get_timing(struct device_node *np,
>> +		struct dram_timing *timing)
>> +{
>> +	int ret;
>> +
>> +	ret = of_property_read_u32(np, "ddr3_speed_bin",
>> +				   &timing->ddr3_speed_bin);
>> +	ret |= of_property_read_u32(np, "pd_idle", &timing->pd_idle);
>> +	ret |= of_property_read_u32(np, "sr_idle", &timing->sr_idle);
>> +	ret |= of_property_read_u32(np, "sr_mc_gate_idle",
>> +				    &timing->sr_mc_gate_idle);
>> +	ret |= of_property_read_u32(np, "srpd_lite_idle",
>> +				    &timing->srpd_lite_idle);
>> +	ret |= of_property_read_u32(np, "standby_idle", &timing->standby_idle);
>> +	ret |= of_property_read_u32(np, "dram_dll_dis_freq",
>> +				    &timing->dram_dll_dis_freq);
>> +	ret |= of_property_read_u32(np, "phy_dll_dis_freq",
>> +				    &timing->phy_dll_dis_freq);
>> +	ret |= of_property_read_u32(np, "ddr3_odt_dis_freq",
>> +				    &timing->ddr3_odt_dis_freq);
>> +	ret |= of_property_read_u32(np, "ddr3_drv", &timing->ddr3_drv);
>> +	ret |= of_property_read_u32(np, "ddr3_odt", &timing->ddr3_odt);
>> +	ret |= of_property_read_u32(np, "phy_ddr3_ca_drv",
>> +				    &timing->phy_ddr3_ca_drv);
>> +	ret |= of_property_read_u32(np, "phy_ddr3_dq_drv",
>> +				    &timing->phy_ddr3_dq_drv);
>> +	ret |= of_property_read_u32(np, "phy_ddr3_odt", &timing->phy_ddr3_odt);
>> +	ret |= of_property_read_u32(np, "lpddr3_odt_dis_freq",
>> +				    &timing->lpddr3_odt_dis_freq);
>> +	ret |= of_property_read_u32(np, "lpddr3_drv", &timing->lpddr3_drv);
>> +	ret |= of_property_read_u32(np, "lpddr3_odt", &timing->lpddr3_odt);
>> +	ret |= of_property_read_u32(np, "phy_lpddr3_ca_drv",
>> +				    &timing->phy_lpddr3_ca_drv);
>> +	ret |= of_property_read_u32(np, "phy_lpddr3_dq_drv",
>> +				    &timing->phy_lpddr3_dq_drv);
>> +	ret |= of_property_read_u32(np, "phy_lpddr3_odt",
>> +				    &timing->phy_lpddr3_odt);
>> +	ret |= of_property_read_u32(np, "lpddr4_odt_dis_freq",
>> +				    &timing->lpddr4_odt_dis_freq);
>> +	ret |= of_property_read_u32(np, "lpddr4_drv",
>> +				    &timing->lpddr4_drv);
>> +	ret |= of_property_read_u32(np, "lpddr4_dq_odt",
>> +				    &timing->lpddr4_dq_odt);
>> +	ret |= of_property_read_u32(np, "lpddr4_ca_odt",
>> +				    &timing->lpddr4_ca_odt);
>> +	ret |= of_property_read_u32(np, "phy_lpddr4_ca_drv",
>> +				    &timing->phy_lpddr4_ca_drv);
>> +	ret |= of_property_read_u32(np, "phy_lpddr4_ck_cs_drv",
>> +				    &timing->phy_lpddr4_ck_cs_drv);
>> +	ret |= of_property_read_u32(np, "phy_lpddr4_dq_drv",
>> +				    &timing->phy_lpddr4_dq_drv);
>> +	ret |= of_property_read_u32(np, "phy_lpddr4_odt",
>> +				    &timing->phy_lpddr4_odt);
>> +
>> +	return ret;
>> +}
>> +
>> +static struct dram_timing *of_get_ddr_timings(struct device *dev,
>> +					      struct device_node *np)
>> +{
>> +	struct dram_timing	*timing = NULL;
>> +	struct device_node	*np_tim;
>> +
>> +	np_tim = of_parse_phandle(np, "ddr_timing", 0);
> As I already mentioned, you need to add the detailed documetation about the property.
>
>> +	if (np_tim) {
>> +		timing = devm_kzalloc(dev, sizeof(*timing), GFP_KERNEL);
>> +		if (!timing)
>> +			goto err;
>> +		if (of_do_get_timing(np_tim, timing)) {
> I recommend that you better to squash following two functions
> because of_do_get_timing() is not used on other point of this driver.
> - of_do_get_timing()
> - of_get_ddr_timings()
>
>> +			devm_kfree(dev, timing);
>> +			goto err;
>> +		}
> You're missing the 'of_node_put'.
> 		of_node_put(np_tim);
>
>> +		return timing;
>> +	}
>> +
>> +err:
>> +	if (timing) {
>> +		devm_kfree(dev, timing);
>> +		timing = NULL;
>> +	}
>> +
> ditto.
> 	of_node_put(np_tim);
>
>> +	return timing;
>> +}
>> +
>> +static int rk3399_dmcfreq_probe(struct platform_device *pdev)
>> +{
>> +	struct arm_smccc_res res;
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *np = pdev->dev.of_node;
>> +	uint64_t param = 0;
>> +	struct rk3399_dmcfreq *data;
>> +	int ret, irq, index;
>> +	uint32_t *timing;
>> +
>> +	irq = platform_get_irq(pdev, 0);
>> +	if (irq < 0) {
>> +		dev_err(&pdev->dev, "no dmc irq resource\n");
> We need to maintain the consistent expression for error log.
> 		dev_err(&pdev->dev, ""Cannot get the interrupt resource"\n");
> 		
>> +		return -EINVAL;
>> +	}
>> +	data = devm_kzalloc(dev, sizeof(struct rk3399_dmcfreq), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	mutex_init(&data->lock);
>> +
>> +	data->vdd_center = devm_regulator_get(dev, "center");
>> +	if (IS_ERR(data->vdd_center)) {
>> +		dev_err(dev, "Cannot get the regulator \"center\"\n");
>> +		return PTR_ERR(data->vdd_center);
>> +	}
>> +
>> +	data->dmc_clk = devm_clk_get(dev, "dmc_clk");
>> +	if (IS_ERR(data->dmc_clk)) {
>> +		dev_err(dev, "Cannot get the clk dmc_clk\n");
>> +		return PTR_ERR(data->dmc_clk);
>> +	};
>> +
>> +	data->irq = irq;
>> +	ret = devm_request_irq(dev, irq, rk3399_dmc_irq, 0,
>> +			       dev_name(dev), data);
>> +	if (ret) {
>> +		dev_err(dev, "failed to request dmc irq: %d\n", ret);
> ditto. "Failed to request the dmc irq" or "Cannot request the dmc irq"
>
>> +		return ret;
>> +	}
>> +
>> +	/* get dram timing and pass it to bl31 */
> I want to add the more detailed description why this code is necessary.
> Because the SMC is usually black box. So, If you add the more explanation,
> it help people to understand this code.
>
>> +	data->timing = of_get_ddr_timings(dev, np);
>> +	if (data->timing) {
>> +		timing = (uint32_t *)data->timing;
>> +		for (index = 0; index < (sizeof(struct dram_timing) / 4);
>> +		     index++) {
> You better to use following code. It is better way to use
> the only one line for 'for' statement.
>
> 		size = sizeof(struct dram_timing) / 4;
> 		for (index = 0; index < size; index++) {
>
>> +			param = index;
>> +			param = param << 32 | *timing++;
>> +			arm_smccc_smc(SIP_DDR_FREQ, param, 0,
>> +				      CONFIG_DRAM_SET_PARAM, 0, 0, 0, 0, &res);
>> +			if (res.a0) {
>> +				dev_err(dev, "failed to set dram param: %ld\n",
>> +					res.a0);
>> +				return -EINVAL;
>> +			}
>> +			param = 0;
>> +		}
>> +	}
>> +
>> +	arm_smccc_smc(SIP_DDR_FREQ, 0, 0, CONFIG_DRAM_INIT,
>> +		      0, 0, 0, 0, &res);
> ditto. You need to add the description.
>
>> +
>> +	init_waitqueue_head(&data->wait_dcf_queue);
>> +	data->wait_dcf_flag = 0;
>> +
>> +	data->edev = devfreq_event_get_edev_by_phandle(dev, 0);
>> +	if (IS_ERR(data->edev))
>> +		return -EPROBE_DEFER;
>> +
>> +	ret = devfreq_event_enable_edev(data->edev);
>> +	if (ret < 0) {
>> +		dev_err(dev, "failed to enable devfreq-event devices\n");
> s/failed/Failed. Use a capital letter for the first char.
>
>> +		return ret;
>> +	}
>> +
>> +	/*
>> +	 * We add a devfreq driver to our parent since it has a device tree node
>> +	 * with operating points.
>> +	 */
> You should wrap the rcu lock before calling the dev_pm_opp_of_add_table().
>
>> +	if (dev_pm_opp_of_add_table(dev)) {
>> +		dev_err(dev, "Invalid operating-points in device tree.\n");
>> +		return -EINVAL;
>> +	}
>> +	of_property_read_u32(np, "upthreshold",
>> +			     &data->ondemand_data.upthreshold);
>> +	of_property_read_u32(np, "downdifferential",
>> +			     &data->ondemand_data.downdifferential);
> Need one blank line.
> Maybe this code to get the properfy for ondemand governor,
> the devfreq will support in the future.
>
>> +	data->rate = clk_get_rate(data->dmc_clk);
>> +	rk3399_devfreq_dmc_profile.initial_freq = data->rate;
> Need one blank line.
>
>> +	data->devfreq = devfreq_add_device(dev,
>> +					   &rk3399_devfreq_dmc_profile,
>> +					   "simple_ondemand",
>> +					   &data->ondemand_data);
>> +	if (IS_ERR(data->devfreq))
>> +		return PTR_ERR(data->devfreq);
>> +	devfreq_register_opp_notifier(dev, data->devfreq);
> Use the devm_devfreq_register_opp_notifier().
>
>> +
>> +	data->dev = dev;
>> +	platform_set_drvdata(pdev, data);
>> +
>> +	return 0;
>> +}
>> +
>> +static int rk3399_dmcfreq_remove(struct platform_device *pdev)
>> +{
>> +	struct rk3399_dmcfreq *dmcfreq = platform_get_drvdata(pdev);
>> +
>> +	devfreq_remove_device(dmcfreq->devfreq);
>> +	regulator_put(dmcfreq->vdd_center);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id rk3399dmc_devfreq_of_match[] = {
>> +	{ .compatible = "rockchip,rk3399-dmc" },
>> +	{ },
>> +};
>> +
>> +static struct platform_driver rk3399_dmcfreq_driver = {
>> +	.probe	= rk3399_dmcfreq_probe,
>> +	.remove	= rk3399_dmcfreq_remove,
>> +	.driver = {
>> +		.name	= "rk3399-dmc-freq",
>> +		.pm	= &rk3399_dmcfreq_pm,
>> +		.of_match_table = rk3399dmc_devfreq_of_match,
>> +	},
>> +};
>> +module_platform_driver(rk3399_dmcfreq_driver);
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("RK3399 dmcfreq driver with devfreq framework");
> You need to add the MODULE_AUTHOR information.
>
> Regards,
> Chanwoo Choi
>
>
>

-- 
Lin Huang



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 6/7] PM / devfreq: rockchip: add devfreq driver for rk3399 dmc

[Chanwoo Choi]

Hi Lin,

On the next version, I'd like you to add the 'linux-pm@vger.kernel.org'
because devfreq is a subsystem of power management.

On 2016년 08월 02일 10:03, hl wrote:
> Hi Chanwoo Choi,
> 
>     Thanks for reviewing so carefully. And i have some question:
> 
> On 2016年08月01日 18:28, Chanwoo Choi wrote:
>> Hi Lin,
>>
>> As I mentioned on patch5, you better to make the documentation as following:
>> - Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
>> And, I add the comments.
>>
>>
>> On 2016년 07월 29일 16:57, Lin Huang wrote:
>>> base on dfi result, we do ddr frequency scaling, register
>>> dmc driver to devfreq framework, and use simple-ondemand
>>> policy.
>>>
>>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>>> ---
>>> Changes in v4:
>>> - use arm_smccc_smc() function talk to bl31
>>> - delete rockchip_dmc.c file and config
>>> - delete dmc_notify
>>> - adjust probe order
>>>   Changes in v3:
>>> - operate dram setting through sip call
>>> - imporve set rate flow
>>>
>>> Changes in v2:
>>> - None
>>>   Changes in v1:
>>> - move dfi controller to event
>>> - fix set voltage sequence when set rate fail
>>> - change Kconfig type from tristate to bool
>>> - move unuse EXPORT_SYMBOL_GPL()
>>>
>>>   drivers/devfreq/Kconfig               |   1 +
>>>   drivers/devfreq/Makefile              |   1 +
>>>   drivers/devfreq/rockchip/Kconfig      |   8 +
>>>   drivers/devfreq/rockchip/Makefile     |   1 +
>>>   drivers/devfreq/rockchip/rk3399_dmc.c | 473 ++++++++++++++++++++++++++++++++++
>>>   5 files changed, 484 insertions(+)
>>>   create mode 100644 drivers/devfreq/rockchip/Kconfig
>>>   create mode 100644 drivers/devfreq/rockchip/Makefile
>>>   create mode 100644 drivers/devfreq/rockchip/rk3399_dmc.c
>>>

[snip]

>>> +
>>> +static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
>>> +                 u32 flags)
>>> +{
>>> +    struct platform_device *pdev = container_of(dev, struct platform_device,
>>> +                            dev);
>>> +    struct rk3399_dmcfreq *dmcfreq = platform_get_drvdata(pdev);
>> You can use the 'dev_get_drvdata()' to simplify it instead of 'platform_get_drvdata()'.
>>
>>     struct rk3399_dmcfreq *dmcfreq = dev_get_drvdata(dev);
>>
>>> +    struct dev_pm_opp *opp;
>>> +    unsigned long old_clk_rate = dmcfreq->rate;
>>> +    unsigned long target_volt, target_rate;
>>> +    int err;
>>> +
>>> +    rcu_read_lock();
>>> +    opp = devfreq_recommended_opp(dev, freq, flags);
>>> +    if (IS_ERR(opp)) {
>>> +        rcu_read_unlock();
>>> +        return PTR_ERR(opp);
>>> +    }
>>> +
>>> +    target_rate = dev_pm_opp_get_freq(opp);
>>> +    target_volt = dev_pm_opp_get_voltage(opp);
>>> +    opp = devfreq_recommended_opp(dev, &dmcfreq->rate, flags);
>>> +    if (IS_ERR(opp)) {
>>> +        rcu_read_unlock();
>>> +        return PTR_ERR(opp);
>>> +    }
>>> +    dmcfreq->volt = dev_pm_opp_get_voltage(opp);
>> If you add the 'curr_opp' variable to struct rk3399_dmcfreq,
>> you can remove the calling of devfreq_recommended_opp().
>>     dmcfreq->rate = dev_pm_opp_get_freq(dmcfreq->curr_opp);
>>     dmcfreq->volt = dev_pm_opp_get_freq(dmcfreq->curr_opp);
>>
>> Because the current rate and voltage is already decided on previous polling cycle,
>> So we don't need to get the opp with devfreq_recommended_opp().

> I prefer the way now use, since we get the dmcfreq->rate use clk_get_rate() after,
> Base on that,  i do not care the set_rate success or fail. use curr_opp i need to
> care about set_rate status, when fail, i must set some rate, when success i must
> set other rate.

I think that it is not good to get the alrady decided opp
by devfreq_recommended_opp(). Usually, devfreq_recommended_opp() is used
to get the proper opp which get the close frequency (dmcfreq->rate).

Also, When finishing the rk3399_dmcfreq_target(), the rk3399_dmc.c
have to know the current opp or rate without any finding sequence.
The additional finding procedure is un-needed.

>>> +    rcu_read_unlock();
>>> +
>>> +    if (dmcfreq->rate == target_rate)
>>> +        return 0;
>>> +
>>> +    mutex_lock(&dmcfreq->lock);
>>> +
>>> +    /*
>>> +     * if frequency scaling from low to high, adjust voltage first;
>>> +     * if frequency scaling from high to low, adjuset frequency first;
>>> +     */
>> s/adjuset/adjust
>>
>> I recommend that you use a captital letter for first character and use the '.'
>> instead of ';'.
>>
>>> +    if (old_clk_rate < target_rate) {
>>> +        err = regulator_set_voltage(dmcfreq->vdd_center, target_volt,
>>> +                        target_volt);
>>> +        if (err) {
>>> +            dev_err(dev, "Unable to set vol %lu\n", target_volt);
>> To readability, you better to use the corrent word to pass the precise the log message.
>> - s/vol/voltage
>>
>> And, this patch uses the 'Unable to' or 'Cannot' to show the error log.
>> I recommend that you use the consistent expression if there is not any specific reason.
>>
>>     dev_err(dev, "Cannot set the voltage %lu uV\n", target_volt);
>>
>>> +            goto out;
>>> +        }
>>> +    }
>>> +    dmcfreq->wait_dcf_flag = 1;
>>> +    err = clk_set_rate(dmcfreq->dmc_clk, target_rate);
>>> +    if (err) {
>>> +        dev_err(dev,
>>> +            "Unable to set freq %lu. Current freq %lu. Error %d\n",
>>> +            target_rate, old_clk_rate, err);
>>     dev_err(dev, "Cannot set the frequency %lu (%d)\n", target_rate, err);
>>
>>> +        regulator_set_voltage(dmcfreq->vdd_center, dmcfreq->volt,
>>> +                      dmcfreq->volt);
>>> +        goto out;
>>> +    }
>>> +
>>> +    /*
>>> +     * wait until bcf irq happen, it means freq scaling finish in bl31,
>> ditto.
>>
>>> +     * use 100ms as timeout time
>> s/time/time.
>>
>>> +     */
>>> +    if (!wait_event_timeout(dmcfreq->wait_dcf_queue,
>>> +                !dmcfreq->wait_dcf_flag, HZ / 10))
>>> +        dev_warn(dev, "Timeout waiting for dcf irq\n");
>> If the timeout happen, are there any problem?

> When timeout happen , may be we miss interrupt, but it do not affect this
> process, since we will check the rate whether success later.

OK. But I'd like you to modify the warning message.

One more thing, is the dcf interrupt related to the change of clock rate?
When the clock rate is changed, the dcf interrupt happen?

>> After setting the frequency and voltage, store the current opp entry on .curr_opp.
>>     dmcfreq->curr_opp = opp;
>>
>>> +    /*
>>> +     * check the dpll rate
>>> +     * there only two result we will get,
>>> +     * 1. ddr frequency scaling fail, we still get the old rate
>>> +     * 2, ddr frequency scaling sucessful, we get the rate we set
>>> +     */
>>> +    dmcfreq->rate = clk_get_rate(dmcfreq->dmc_clk);
>>> +
>>> +    /* if get the incorrect rate, set voltage to old value */
>>> +    if (dmcfreq->rate != target_rate) {
>>> +        dev_err(dev, "get wrong ddr frequency, Request freq %lu,\
>>> +            Current freq %lu\n", target_rate, dmcfreq->rate);
>>> +        regulator_set_voltage(dmcfreq->vdd_center, dmcfreq->volt,
>>> +                      dmcfreq->volt);
>> [Without force, it is just my opion about this code:]
>> I think that this checking code it is un-needed.
>> If this case occur, the rk3399_dmc.c never set the specific frequency
>> because the rk3399 clock don't support the specific frequency for rk3399 dmc.
>>
>> If you want to set the correct frequency,
>> When verifying the rk3399 dmc driver, you should check the rk3399 clock driver.
>>
>> Basically, if the the clock driver don't support the correct same frequency ,
>> CCF(Common Clock Framework) set the close frequency. It is not a bad thing.

> May be i should remove the regulator_set_voltage() here, but still need to
> check whether we get the right frequency, since if we do not get the right frequency,

When calling clk_set_rate(), the final frequency only depend on the rk3399 clock driver.
But, if you want to check the new rate, I think that you should move this code
right after clk_set_rate() when there is any dependency of dcf interrupt.

> we should send a warn message, to remind that maybe you pass a frequency which
> do not support in bl31.

Also, I'd like you to explain the 'bl31' and add the description on next version.

[snip]

Regards,
Chanwoo Choi

Re: [PATCH v4 6/7] PM / devfreq: rockchip: add devfreq driver for rk3399 dmc

[hl]

Hi Chanwoo Choi,
On 2016年08月02日 12:21, Chanwoo Choi wrote:
> Hi Lin,
>
> On the next version, I'd like you to add the 'linux-pm@vger.kernel.org'
> because devfreq is a subsystem of power management.
Sure, will do it next version.
> On 2016년 08월 02일 10:03, hl wrote:
>> Hi Chanwoo Choi,
>>
>>      Thanks for reviewing so carefully. And i have some question:
>>
>> On 2016年08月01日 18:28, Chanwoo Choi wrote:
>>> Hi Lin,
>>>
>>> As I mentioned on patch5, you better to make the documentation as following:
>>> - Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
>>> And, I add the comments.
>>>
>>>
>>> On 2016년 07월 29일 16:57, Lin Huang wrote:
>>>> base on dfi result, we do ddr frequency scaling, register
>>>> dmc driver to devfreq framework, and use simple-ondemand
>>>> policy.
>>>>
>>>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>>>> ---
>>>> Changes in v4:
>>>> - use arm_smccc_smc() function talk to bl31
>>>> - delete rockchip_dmc.c file and config
>>>> - delete dmc_notify
>>>> - adjust probe order
>>>>    Changes in v3:
>>>> - operate dram setting through sip call
>>>> - imporve set rate flow
>>>>
>>>> Changes in v2:
>>>> - None
>>>>    Changes in v1:
>>>> - move dfi controller to event
>>>> - fix set voltage sequence when set rate fail
>>>> - change Kconfig type from tristate to bool
>>>> - move unuse EXPORT_SYMBOL_GPL()
>>>>
>>>>    drivers/devfreq/Kconfig               |   1 +
>>>>    drivers/devfreq/Makefile              |   1 +
>>>>    drivers/devfreq/rockchip/Kconfig      |   8 +
>>>>    drivers/devfreq/rockchip/Makefile     |   1 +
>>>>    drivers/devfreq/rockchip/rk3399_dmc.c | 473 ++++++++++++++++++++++++++++++++++
>>>>    5 files changed, 484 insertions(+)
>>>>    create mode 100644 drivers/devfreq/rockchip/Kconfig
>>>>    create mode 100644 drivers/devfreq/rockchip/Makefile
>>>>    create mode 100644 drivers/devfreq/rockchip/rk3399_dmc.c
>>>>
> [snip]
>
>>>> +
>>>> +static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
>>>> +                 u32 flags)
>>>> +{
>>>> +    struct platform_device *pdev = container_of(dev, struct platform_device,
>>>> +                            dev);
>>>> +    struct rk3399_dmcfreq *dmcfreq = platform_get_drvdata(pdev);
>>> You can use the 'dev_get_drvdata()' to simplify it instead of 'platform_get_drvdata()'.
>>>
>>>      struct rk3399_dmcfreq *dmcfreq = dev_get_drvdata(dev);
>>>
>>>> +    struct dev_pm_opp *opp;
>>>> +    unsigned long old_clk_rate = dmcfreq->rate;
>>>> +    unsigned long target_volt, target_rate;
>>>> +    int err;
>>>> +
>>>> +    rcu_read_lock();
>>>> +    opp = devfreq_recommended_opp(dev, freq, flags);
>>>> +    if (IS_ERR(opp)) {
>>>> +        rcu_read_unlock();
>>>> +        return PTR_ERR(opp);
>>>> +    }
>>>> +
>>>> +    target_rate = dev_pm_opp_get_freq(opp);
>>>> +    target_volt = dev_pm_opp_get_voltage(opp);
>>>> +    opp = devfreq_recommended_opp(dev, &dmcfreq->rate, flags);
>>>> +    if (IS_ERR(opp)) {
>>>> +        rcu_read_unlock();
>>>> +        return PTR_ERR(opp);
>>>> +    }
>>>> +    dmcfreq->volt = dev_pm_opp_get_voltage(opp);
>>> If you add the 'curr_opp' variable to struct rk3399_dmcfreq,
>>> you can remove the calling of devfreq_recommended_opp().
>>>      dmcfreq->rate = dev_pm_opp_get_freq(dmcfreq->curr_opp);
>>>      dmcfreq->volt = dev_pm_opp_get_freq(dmcfreq->curr_opp);
>>>
>>> Because the current rate and voltage is already decided on previous polling cycle,
>>> So we don't need to get the opp with devfreq_recommended_opp().
>> I prefer the way now use, since we get the dmcfreq->rate use clk_get_rate() after,
>> Base on that,  i do not care the set_rate success or fail. use curr_opp i need to
>> care about set_rate status, when fail, i must set some rate, when success i must
>> set other rate.
> I think that it is not good to get the alrady decided opp
> by devfreq_recommended_opp(). Usually, devfreq_recommended_opp() is used
> to get the proper opp which get the close frequency (dmcfreq->rate).
>
> Also, When finishing the rk3399_dmcfreq_target(), the rk3399_dmc.c
> have to know the current opp or rate without any finding sequence.
> The additional finding procedure is un-needed.
>
>>>> +    rcu_read_unlock();
>>>> +
>>>> +    if (dmcfreq->rate == target_rate)
>>>> +        return 0;
>>>> +
>>>> +    mutex_lock(&dmcfreq->lock);
>>>> +
>>>> +    /*
>>>> +     * if frequency scaling from low to high, adjust voltage first;
>>>> +     * if frequency scaling from high to low, adjuset frequency first;
>>>> +     */
>>> s/adjuset/adjust
>>>
>>> I recommend that you use a captital letter for first character and use the '.'
>>> instead of ';'.
>>>
>>>> +    if (old_clk_rate < target_rate) {
>>>> +        err = regulator_set_voltage(dmcfreq->vdd_center, target_volt,
>>>> +                        target_volt);
>>>> +        if (err) {
>>>> +            dev_err(dev, "Unable to set vol %lu\n", target_volt);
>>> To readability, you better to use the corrent word to pass the precise the log message.
>>> - s/vol/voltage
>>>
>>> And, this patch uses the 'Unable to' or 'Cannot' to show the error log.
>>> I recommend that you use the consistent expression if there is not any specific reason.
>>>
>>>      dev_err(dev, "Cannot set the voltage %lu uV\n", target_volt);
>>>
>>>> +            goto out;
>>>> +        }
>>>> +    }
>>>> +    dmcfreq->wait_dcf_flag = 1;
>>>> +    err = clk_set_rate(dmcfreq->dmc_clk, target_rate);
>>>> +    if (err) {
>>>> +        dev_err(dev,
>>>> +            "Unable to set freq %lu. Current freq %lu. Error %d\n",
>>>> +            target_rate, old_clk_rate, err);
>>>      dev_err(dev, "Cannot set the frequency %lu (%d)\n", target_rate, err);
>>>
>>>> +        regulator_set_voltage(dmcfreq->vdd_center, dmcfreq->volt,
>>>> +                      dmcfreq->volt);
>>>> +        goto out;
>>>> +    }
>>>> +
>>>> +    /*
>>>> +     * wait until bcf irq happen, it means freq scaling finish in bl31,
>>> ditto.
>>>
>>>> +     * use 100ms as timeout time
>>> s/time/time.
>>>
>>>> +     */
>>>> +    if (!wait_event_timeout(dmcfreq->wait_dcf_queue,
>>>> +                !dmcfreq->wait_dcf_flag, HZ / 10))
>>>> +        dev_warn(dev, "Timeout waiting for dcf irq\n");
>>> If the timeout happen, are there any problem?
>> When timeout happen , may be we miss interrupt, but it do not affect this
>> process, since we will check the rate whether success later.
> OK. But I'd like you to modify the warning message.
>
> One more thing, is the dcf interrupt related to the change of clock rate?
> When the clock rate is changed, the dcf interrupt happen?
Yes, when clock rate changed sucessful, it will trigger a irq in bl31.
>
>>> After setting the frequency and voltage, store the current opp entry on .curr_opp.
>>>      dmcfreq->curr_opp = opp;
>>>
>>>> +    /*
>>>> +     * check the dpll rate
>>>> +     * there only two result we will get,
>>>> +     * 1. ddr frequency scaling fail, we still get the old rate
>>>> +     * 2, ddr frequency scaling sucessful, we get the rate we set
>>>> +     */
>>>> +    dmcfreq->rate = clk_get_rate(dmcfreq->dmc_clk);
>>>> +
>>>> +    /* if get the incorrect rate, set voltage to old value */
>>>> +    if (dmcfreq->rate != target_rate) {
>>>> +        dev_err(dev, "get wrong ddr frequency, Request freq %lu,\
>>>> +            Current freq %lu\n", target_rate, dmcfreq->rate);
>>>> +        regulator_set_voltage(dmcfreq->vdd_center, dmcfreq->volt,
>>>> +                      dmcfreq->volt);
>>> [Without force, it is just my opion about this code:]
>>> I think that this checking code it is un-needed.
>>> If this case occur, the rk3399_dmc.c never set the specific frequency
>>> because the rk3399 clock don't support the specific frequency for rk3399 dmc.
>>>
>>> If you want to set the correct frequency,
>>> When verifying the rk3399 dmc driver, you should check the rk3399 clock driver.
>>>
>>> Basically, if the the clock driver don't support the correct same frequency ,
>>> CCF(Common Clock Framework) set the close frequency. It is not a bad thing.
>> May be i should remove the regulator_set_voltage() here, but still need to
>> check whether we get the right frequency, since if we do not get the right frequency,
> When calling clk_set_rate(), the final frequency only depend on the rk3399 clock driver.
> But, if you want to check the new rate, I think that you should move this code
> right after clk_set_rate() when there is any dependency of dcf interrupt.
it should be after the wait_event, since it indicate the clk_set_rate 
finish,
>
>> we should send a warn message, to remind that maybe you pass a frequency which
>> do not support in bl31.
> Also, I'd like you to explain the 'bl31' and add the description on next version.
>
> [snip]
>
> Regards,
> Chanwoo Choi
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip


-- 
Lin Huang

Re: [PATCH v4 6/7] PM / devfreq: rockchip: add devfreq driver for rk3399 dmc

[Chanwoo Choi]

Hi Lin,

On 2016년 08월 03일 16:38, hl wrote:
> 
> Hi Chanwoo Choi,
> On 2016年08月02日 12:21, Chanwoo Choi wrote:
>> Hi Lin,
>>
>> On the next version, I'd like you to add the 'linux-pm@vger.kernel.org'
>> because devfreq is a subsystem of power management.
> Sure, will do it next version.
>> On 2016년 08월 02일 10:03, hl wrote:
>>> Hi Chanwoo Choi,
>>>
>>>      Thanks for reviewing so carefully. And i have some question:
>>>
>>> On 2016年08月01日 18:28, Chanwoo Choi wrote:
>>>> Hi Lin,
>>>>
>>>> As I mentioned on patch5, you better to make the documentation as following:
>>>> - Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
>>>> And, I add the comments.
>>>>
>>>>
>>>> On 2016년 07월 29일 16:57, Lin Huang wrote:
>>>>> base on dfi result, we do ddr frequency scaling, register
>>>>> dmc driver to devfreq framework, and use simple-ondemand
>>>>> policy.
>>>>>
>>>>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>>>>> ---
>>>>> Changes in v4:
>>>>> - use arm_smccc_smc() function talk to bl31
>>>>> - delete rockchip_dmc.c file and config
>>>>> - delete dmc_notify
>>>>> - adjust probe order
>>>>>    Changes in v3:
>>>>> - operate dram setting through sip call
>>>>> - imporve set rate flow
>>>>>
>>>>> Changes in v2:
>>>>> - None
>>>>>    Changes in v1:
>>>>> - move dfi controller to event
>>>>> - fix set voltage sequence when set rate fail
>>>>> - change Kconfig type from tristate to bool
>>>>> - move unuse EXPORT_SYMBOL_GPL()
>>>>>
>>>>>    drivers/devfreq/Kconfig               |   1 +
>>>>>    drivers/devfreq/Makefile              |   1 +
>>>>>    drivers/devfreq/rockchip/Kconfig      |   8 +
>>>>>    drivers/devfreq/rockchip/Makefile     |   1 +
>>>>>    drivers/devfreq/rockchip/rk3399_dmc.c | 473 ++++++++++++++++++++++++++++++++++
>>>>>    5 files changed, 484 insertions(+)
>>>>>    create mode 100644 drivers/devfreq/rockchip/Kconfig
>>>>>    create mode 100644 drivers/devfreq/rockchip/Makefile
>>>>>    create mode 100644 drivers/devfreq/rockchip/rk3399_dmc.c
>>>>>
>> [snip]
>>
>>>>> +
>>>>> +static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
>>>>> +                 u32 flags)
>>>>> +{
>>>>> +    struct platform_device *pdev = container_of(dev, struct platform_device,
>>>>> +                            dev);
>>>>> +    struct rk3399_dmcfreq *dmcfreq = platform_get_drvdata(pdev);
>>>> You can use the 'dev_get_drvdata()' to simplify it instead of 'platform_get_drvdata()'.
>>>>
>>>>      struct rk3399_dmcfreq *dmcfreq = dev_get_drvdata(dev);
>>>>
>>>>> +    struct dev_pm_opp *opp;
>>>>> +    unsigned long old_clk_rate = dmcfreq->rate;
>>>>> +    unsigned long target_volt, target_rate;
>>>>> +    int err;
>>>>> +
>>>>> +    rcu_read_lock();
>>>>> +    opp = devfreq_recommended_opp(dev, freq, flags);
>>>>> +    if (IS_ERR(opp)) {
>>>>> +        rcu_read_unlock();
>>>>> +        return PTR_ERR(opp);
>>>>> +    }
>>>>> +
>>>>> +    target_rate = dev_pm_opp_get_freq(opp);
>>>>> +    target_volt = dev_pm_opp_get_voltage(opp);
>>>>> +    opp = devfreq_recommended_opp(dev, &dmcfreq->rate, flags);
>>>>> +    if (IS_ERR(opp)) {
>>>>> +        rcu_read_unlock();
>>>>> +        return PTR_ERR(opp);
>>>>> +    }
>>>>> +    dmcfreq->volt = dev_pm_opp_get_voltage(opp);
>>>> If you add the 'curr_opp' variable to struct rk3399_dmcfreq,
>>>> you can remove the calling of devfreq_recommended_opp().
>>>>      dmcfreq->rate = dev_pm_opp_get_freq(dmcfreq->curr_opp);
>>>>      dmcfreq->volt = dev_pm_opp_get_freq(dmcfreq->curr_opp);
>>>>
>>>> Because the current rate and voltage is already decided on previous polling cycle,
>>>> So we don't need to get the opp with devfreq_recommended_opp().
>>> I prefer the way now use, since we get the dmcfreq->rate use clk_get_rate() after,
>>> Base on that,  i do not care the set_rate success or fail. use curr_opp i need to
>>> care about set_rate status, when fail, i must set some rate, when success i must
>>> set other rate.
>> I think that it is not good to get the alrady decided opp
>> by devfreq_recommended_opp(). Usually, devfreq_recommended_opp() is used
>> to get the proper opp which get the close frequency (dmcfreq->rate).
>>
>> Also, When finishing the rk3399_dmcfreq_target(), the rk3399_dmc.c
>> have to know the current opp or rate without any finding sequence.
>> The additional finding procedure is un-needed.
>>
>>>>> +    rcu_read_unlock();
>>>>> +
>>>>> +    if (dmcfreq->rate == target_rate)
>>>>> +        return 0;
>>>>> +
>>>>> +    mutex_lock(&dmcfreq->lock);
>>>>> +
>>>>> +    /*
>>>>> +     * if frequency scaling from low to high, adjust voltage first;
>>>>> +     * if frequency scaling from high to low, adjuset frequency first;
>>>>> +     */
>>>> s/adjuset/adjust
>>>>
>>>> I recommend that you use a captital letter for first character and use the '.'
>>>> instead of ';'.
>>>>
>>>>> +    if (old_clk_rate < target_rate) {
>>>>> +        err = regulator_set_voltage(dmcfreq->vdd_center, target_volt,
>>>>> +                        target_volt);
>>>>> +        if (err) {
>>>>> +            dev_err(dev, "Unable to set vol %lu\n", target_volt);
>>>> To readability, you better to use the corrent word to pass the precise the log message.
>>>> - s/vol/voltage
>>>>
>>>> And, this patch uses the 'Unable to' or 'Cannot' to show the error log.
>>>> I recommend that you use the consistent expression if there is not any specific reason.
>>>>
>>>>      dev_err(dev, "Cannot set the voltage %lu uV\n", target_volt);
>>>>
>>>>> +            goto out;
>>>>> +        }
>>>>> +    }
>>>>> +    dmcfreq->wait_dcf_flag = 1;
>>>>> +    err = clk_set_rate(dmcfreq->dmc_clk, target_rate);
>>>>> +    if (err) {
>>>>> +        dev_err(dev,
>>>>> +            "Unable to set freq %lu. Current freq %lu. Error %d\n",
>>>>> +            target_rate, old_clk_rate, err);
>>>>      dev_err(dev, "Cannot set the frequency %lu (%d)\n", target_rate, err);
>>>>
>>>>> +        regulator_set_voltage(dmcfreq->vdd_center, dmcfreq->volt,
>>>>> +                      dmcfreq->volt);
>>>>> +        goto out;
>>>>> +    }
>>>>> +
>>>>> +    /*
>>>>> +     * wait until bcf irq happen, it means freq scaling finish in bl31,
>>>> ditto.
>>>>
>>>>> +     * use 100ms as timeout time
>>>> s/time/time.
>>>>
>>>>> +     */
>>>>> +    if (!wait_event_timeout(dmcfreq->wait_dcf_queue,
>>>>> +                !dmcfreq->wait_dcf_flag, HZ / 10))
>>>>> +        dev_warn(dev, "Timeout waiting for dcf irq\n");
>>>> If the timeout happen, are there any problem?
>>> When timeout happen , may be we miss interrupt, but it do not affect this
>>> process, since we will check the rate whether success later.
>> OK. But I'd like you to modify the warning message.
>>
>> One more thing, is the dcf interrupt related to the change of clock rate?
>> When the clock rate is changed, the dcf interrupt happen?
> Yes, when clock rate changed sucessful, it will trigger a irq in bl31.

OK.

>>
>>>> After setting the frequency and voltage, store the current opp entry on .curr_opp.
>>>>      dmcfreq->curr_opp = opp;
>>>>
>>>>> +    /*
>>>>> +     * check the dpll rate
>>>>> +     * there only two result we will get,
>>>>> +     * 1. ddr frequency scaling fail, we still get the old rate
>>>>> +     * 2, ddr frequency scaling sucessful, we get the rate we set
>>>>> +     */
>>>>> +    dmcfreq->rate = clk_get_rate(dmcfreq->dmc_clk);
>>>>> +
>>>>> +    /* if get the incorrect rate, set voltage to old value */
>>>>> +    if (dmcfreq->rate != target_rate) {
>>>>> +        dev_err(dev, "get wrong ddr frequency, Request freq %lu,\
>>>>> +            Current freq %lu\n", target_rate, dmcfreq->rate);
>>>>> +        regulator_set_voltage(dmcfreq->vdd_center, dmcfreq->volt,
>>>>> +                      dmcfreq->volt);
>>>> [Without force, it is just my opion about this code:]
>>>> I think that this checking code it is un-needed.
>>>> If this case occur, the rk3399_dmc.c never set the specific frequency
>>>> because the rk3399 clock don't support the specific frequency for rk3399 dmc.
>>>>
>>>> If you want to set the correct frequency,
>>>> When verifying the rk3399 dmc driver, you should check the rk3399 clock driver.
>>>>
>>>> Basically, if the the clock driver don't support the correct same frequency ,
>>>> CCF(Common Clock Framework) set the close frequency. It is not a bad thing.
>>> May be i should remove the regulator_set_voltage() here, but still need to
>>> check whether we get the right frequency, since if we do not get the right frequency,
>> When calling clk_set_rate(), the final frequency only depend on the rk3399 clock driver.
>> But, if you want to check the new rate, I think that you should move this code
>> right after clk_set_rate() when there is any dependency of dcf interrupt.
> it should be after the wait_event, since it indicate the clk_set_rate finish,

OK.

>>
>>> we should send a warn message, to remind that maybe you pass a frequency which
>>> do not support in bl31.
>> Also, I'd like you to explain the 'bl31' and add the description on next version.
>>
>> [snip]
>>
>> Regards,
>> Chanwoo Choi

Regards,
Chanwoo Choi

[PATCH v4 7/7] drm/rockchip: Add dmc notifier in vop driver

[Lin Huang]

when in ddr frequency scaling process, vop can not do
enable or disable operation, since dcf will base on vop vblank
time to do frequency scaling and need to get vop irq if there
have vop enabled. So need register to devfreq notifier, and we can
get the dmc status. Also, when there have two vop enabled, we need
to disable dmc, since dcf only base on one vop vblank time, so the
other panel will flicker when do ddr frequency scaling.

Signed-off-by: Lin Huang <hl@rock-chips.com>
---
Changes in v4:
- register notifier to devfreq_register_notifier
- use DEVFREQ_PRECHANGE and DEVFREQ_POSTCHANGE to get dmc status
- when two vop enable, disable dmc
- when two vop back to one vop, enable dmc

Changes in v3:
- when do vop eanble/disable, dmc will wait until it finish

Changes in v2:
- None

Changes in v1:
- use wait_event instead usleep

 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 124 ++++++++++++++++++++++++++--
 1 file changed, 119 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index c179933..3b251d1 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -12,6 +12,8 @@
  * GNU General Public License for more details.
  */
 
+#include <linux/devfreq.h>
+#include <linux/devfreq-event.h>
 #include <drm/drm.h>
 #include <drm/drmP.h>
 #include <drm/drm_atomic.h>
@@ -127,6 +129,13 @@ struct vop {
 
 	const struct vop_data *data;
 
+	struct devfreq *devfreq;
+	struct devfreq_event_dev *devfreq_event_dev;
+	struct notifier_block dmc_nb;
+	int dmc_in_process;
+	int vop_switch_status;
+	wait_queue_head_t wait_dmc_queue;
+	wait_queue_head_t wait_vop_switch_queue;
 	uint32_t *regsbak;
 	void __iomem *regs;
 
@@ -500,24 +509,60 @@ static void vop_line_flag_irq_disable(struct vop *vop)
 	spin_unlock_irqrestore(&vop->irq_lock, flags);
 }
 
+static int dmc_notify(struct notifier_block *nb, unsigned long event,
+		      void *data)
+{
+	struct vop *vop = container_of(nb, struct vop, dmc_nb);
+
+	if (event == DEVFREQ_PRECHANGE) {
+
+		/*
+		 * check if vop in enable or disable process,
+		 * if yes, wait until it finish, use 200ms as
+		 * timeout.
+		 */
+		if (!wait_event_timeout(vop->wait_vop_switch_queue,
+					!vop->vop_switch_status, HZ / 5))
+			dev_warn(vop->dev,
+				 "Timeout waiting for vop swtich status\n");
+		vop->dmc_in_process = 1;
+	} else if (event == DEVFREQ_POSTCHANGE) {
+		vop->dmc_in_process = 0;
+		wake_up(&vop->wait_dmc_queue);
+	}
+
+	return NOTIFY_OK;
+}
+
 static void vop_enable(struct drm_crtc *crtc)
 {
 	struct vop *vop = to_vop(crtc);
+	int num_enabled_crtc = 0;
 	int ret;
 
 	if (vop->is_enabled)
 		return;
 
+	/*
+	 * if in dmc scaling frequency process, wait until it finishes
+	 * use 100ms as timeout time.
+	 */
+	if (!wait_event_timeout(vop->wait_dmc_queue,
+				!vop->dmc_in_process, HZ / 5))
+		dev_warn(vop->dev,
+			 "Timeout waiting for dmc when vop enable\n");
+
+	vop->vop_switch_status = 1;
 	ret = pm_runtime_get_sync(vop->dev);
 	if (ret < 0) {
 		dev_err(vop->dev, "failed to get pm runtime: %d\n", ret);
-		return;
+		goto err;
 	}
 
 	ret = clk_enable(vop->hclk);
 	if (ret < 0) {
 		dev_err(vop->dev, "failed to enable hclk - %d\n", ret);
-		return;
+		goto err;
 	}
 
 	ret = clk_enable(vop->dclk);
@@ -531,7 +576,6 @@ static void vop_enable(struct drm_crtc *crtc)
 		dev_err(vop->dev, "failed to enable aclk - %d\n", ret);
 		goto err_disable_dclk;
 	}
-
 	/*
 	 * Slave iommu shares power, irq and clock with vop.  It was associated
 	 * automatically with this master device via common driver code.
@@ -560,6 +604,21 @@ static void vop_enable(struct drm_crtc *crtc)
 
 	drm_crtc_vblank_on(crtc);
 
+	vop->vop_switch_status = 0;
+	wake_up(&vop->wait_vop_switch_queue);
+
+	/* check how many vop we use now */
+	drm_for_each_crtc(crtc, vop->drm_dev) {
+		if (crtc->state->enable)
+			num_enabled_crtc++;
+	}
+
+	/* if enable two vop, need to disable dmc */
+	if ((num_enabled_crtc > 1) && vop->devfreq) {
+		if (vop->devfreq_event_dev)
+			devfreq_event_disable_edev(vop->devfreq_event_dev);
+		devfreq_suspend_device(vop->devfreq);
+	}
 	return;
 
 err_disable_aclk:
@@ -568,17 +627,33 @@ err_disable_dclk:
 	clk_disable(vop->dclk);
 err_disable_hclk:
 	clk_disable(vop->hclk);
+err:
+	vop->vop_switch_status = 0;
+	wake_up(&vop->wait_vop_switch_queue);
+	return;
 }
 
 static void vop_crtc_disable(struct drm_crtc *crtc)
 {
 	struct vop *vop = to_vop(crtc);
+	int num_enabled_crtc = 0;
 	int i;
 
 	if (!vop->is_enabled)
 		return;
 
 	/*
+	 * if in dmc scaling frequency process, wait until it finish
+	 * use 100ms as timeout time.
+	 */
+	if (!wait_event_timeout(vop->wait_dmc_queue,
+				!vop->dmc_in_process, HZ / 5))
+		dev_warn(vop->dev,
+			 "Timeout waiting for dmc when vop disable\n");
+
+	vop->vop_switch_status = 1;
+
+	/*
 	 * We need to make sure that all windows are disabled before we
 	 * disable that crtc. Otherwise we might try to scan from a destroyed
 	 * buffer later.
@@ -591,7 +666,6 @@ static void vop_crtc_disable(struct drm_crtc *crtc)
 		VOP_WIN_SET(vop, win, enable, 0);
 		spin_unlock(&vop->reg_lock);
 	}
-
 	drm_crtc_vblank_off(crtc);
 
 	/*
@@ -622,7 +696,6 @@ static void vop_crtc_disable(struct drm_crtc *crtc)
 	 * vop standby complete, so iommu detach is safe.
 	 */
 	rockchip_drm_dma_detach_device(vop->drm_dev, vop->dev);
-
 	clk_disable(vop->dclk);
 	clk_disable(vop->aclk);
 	clk_disable(vop->hclk);
@@ -632,6 +705,25 @@ static void vop_crtc_disable(struct drm_crtc *crtc)
 		vop->psr_enabled = false;
 		schedule_delayed_work(&vop->psr_work, VOP_PSR_SET_DELAY_TIME);
 	}
+
+	vop->vop_switch_status = 0;
+	wake_up(&vop->wait_vop_switch_queue);
+
+	/* check how many vop use now */
+	drm_for_each_crtc(crtc, vop->drm_dev) {
+		if (crtc->state->enable)
+			num_enabled_crtc++;
+	}
+
+	/*
+	 * if num_enabled_crtc = 1 now, it means 2 vop enabled
+	 * change to 1 vop enabled  need to enable dmc again.
+	 */
+	if ((num_enabled_crtc == 1) && vop->devfreq) {
+		if (vop->devfreq_event_dev)
+			devfreq_event_enable_edev(vop->devfreq_event_dev);
+		devfreq_resume_device(vop->devfreq);
+	}
 }
 
 static void vop_plane_destroy(struct drm_plane *plane)
@@ -1583,6 +1675,8 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
 	struct drm_device *drm_dev = data;
 	struct vop *vop;
 	struct resource *res;
+	struct devfreq *devfreq;
+	struct devfreq_event_dev *event_dev;
 	size_t alloc_size;
 	int ret, irq;
 
@@ -1648,6 +1742,26 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
 	vop->psr_enabled = false;
 	INIT_DELAYED_WORK(&vop->psr_work, vop_psr_work);
 
+	init_waitqueue_head(&vop->wait_vop_switch_queue);
+	vop->vop_switch_status = 0;
+	init_waitqueue_head(&vop->wait_dmc_queue);
+	vop->dmc_in_process = 0;
+
+	devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
+	if (IS_ERR(devfreq))
+		goto out;
+
+	vop->devfreq = devfreq;
+	vop->dmc_nb.notifier_call = dmc_notify;
+	devfreq_register_notifier(vop->devfreq, &vop->dmc_nb,
+				  DEVFREQ_TRANSITION_NOTIFIER);
+
+	event_dev = devfreq_event_get_edev_by_phandle(vop->devfreq->dev.parent,
+						      0);
+	if (IS_ERR(event_dev))
+		goto out;
+	vop->devfreq_event_dev = event_dev;
+out:
 	return 0;
 }
 
-- 
1.9.1

Re: [PATCH v4 0/7] rk3399 support ddr frequency scaling

[Chanwoo Choi]

Hi Lin,

On 2016년 07월 29일 16:56, Lin Huang wrote:
> rk3399 platform have dfi controller can monitor ddr load,
> and dcf controller to handle ddr register so we can get the
> right ddr frequency and make ddr controller happy work(which
> will implement in bl31). So we do ddr frequency scaling with
> following flow:
> 
> 	     kernel                                bl31
> 
> 	monitor ddr load
> 		|
> 		|
> 	get_target_rate
> 		|
> 		|           pass rate to bl31
> 	clk_set_rate(ddr) --------------------->run dcf flow
> 		|                                   |
> 		|                                   |
> 	wait dcf interrupt<-------------------trigger dcf interrupt  
> 		|
> 		|
> 	      return
> 
> Lin Huang (6):
>   clk: rockchip: add new clock-type for the ddrclk
>   clk: rockchip: rk3399: add SCLK_DDRCLK ID for ddrc
>   clk: rockchip: rk3399: add ddrc clock support
>   PM / devfreq: event: support rockchip dfi controller
>   PM / devfreq: rockchip: add devfreq driver for rk3399 dmc
>   drm/rockchip: Add dmc notifier in vop driver
> 
> 
> Heiko Stübner (1):
>   clk: rockchip: add clock flag parameter when register pll
> 
> Lin Huang (6):
>   clk: rockchip: add new clock-type for the ddrclk
>   clk: rockchip: rk3399: add SCLK_DDRCLK ID for ddrc
>   clk: rockchip: rk3399: add ddrc clock support
>   PM / devfreq: event: support rockchip dfi controller
>   PM / devfreq: rockchip: add devfreq driver for rk3399 dmc
>   drm/rockchip: Add dmc notifier in vop driver

The cover-letter includes the duplicate list of patches.

Also, I want to test the build test. but, When I apply these patches,
merge conflict happen. Could you give the information about base git repository?

Regards,
Chanwoo Choi


>  drivers/clk/rockchip/Makefile               |   1 +
>  drivers/clk/rockchip/clk-ddr.c              | 146 +++++++++
>  drivers/clk/rockchip/clk-pll.c              |   4 +-
>  drivers/clk/rockchip/clk-rk3399.c           |  19 ++
>  drivers/clk/rockchip/clk.c                  |  11 +-
>  drivers/clk/rockchip/clk.h                  |  29 +-
>  drivers/devfreq/Kconfig                     |   1 +
>  drivers/devfreq/Makefile                    |   1 +
>  drivers/devfreq/event/Kconfig               |   7 +
>  drivers/devfreq/event/Makefile              |   1 +
>  drivers/devfreq/event/rockchip-dfi.c        | 253 +++++++++++++++
>  drivers/devfreq/rockchip/Kconfig            |   8 +
>  drivers/devfreq/rockchip/Makefile           |   1 +
>  drivers/devfreq/rockchip/rk3399_dmc.c       | 473 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 124 +++++++-
>  include/dt-bindings/clock/rk3399-cru.h      |   1 +
>  include/soc/rockchip/rockchip_sip.h         |  27 ++
>  17 files changed, 1098 insertions(+), 9 deletions(-)
>  create mode 100644 drivers/clk/rockchip/clk-ddr.c
>  create mode 100644 drivers/devfreq/event/rockchip-dfi.c
>  create mode 100644 drivers/devfreq/rockchip/Kconfig
>  create mode 100644 drivers/devfreq/rockchip/Makefile
>  create mode 100644 drivers/devfreq/rockchip/rk3399_dmc.c
>  create mode 100644 include/soc/rockchip/rockchip_sip.h
> 


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 0/7] rk3399 support ddr frequency scaling

[hl]

Hi Chanwoo Choi,

     Ah, i am base on 
https://chromium.googlesource.com/chromiumos/third_party/kernel/v4.4,
and forget to rebase on 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git, i 
will fix it in next version.
I am sorry about that. And can you help to review the devfreq patch 
first, if something need to update,
i will do it together. Thanks.

On 2016年08月01日 15:39, Chanwoo Choi wrote:
> Hi Lin,
>
> On 2016년 07월 29일 16:56, Lin Huang wrote:
>> rk3399 platform have dfi controller can monitor ddr load,
>> and dcf controller to handle ddr register so we can get the
>> right ddr frequency and make ddr controller happy work(which
>> will implement in bl31). So we do ddr frequency scaling with
>> following flow:
>>
>> 	     kernel                                bl31
>>
>> 	monitor ddr load
>> 		|
>> 		|
>> 	get_target_rate
>> 		|
>> 		|           pass rate to bl31
>> 	clk_set_rate(ddr) --------------------->run dcf flow
>> 		|                                   |
>> 		|                                   |
>> 	wait dcf interrupt<-------------------trigger dcf interrupt
>> 		|
>> 		|
>> 	      return
>>
>> Lin Huang (6):
>>    clk: rockchip: add new clock-type for the ddrclk
>>    clk: rockchip: rk3399: add SCLK_DDRCLK ID for ddrc
>>    clk: rockchip: rk3399: add ddrc clock support
>>    PM / devfreq: event: support rockchip dfi controller
>>    PM / devfreq: rockchip: add devfreq driver for rk3399 dmc
>>    drm/rockchip: Add dmc notifier in vop driver
>>
>>
>> Heiko Stübner (1):
>>    clk: rockchip: add clock flag parameter when register pll
>>
>> Lin Huang (6):
>>    clk: rockchip: add new clock-type for the ddrclk
>>    clk: rockchip: rk3399: add SCLK_DDRCLK ID for ddrc
>>    clk: rockchip: rk3399: add ddrc clock support
>>    PM / devfreq: event: support rockchip dfi controller
>>    PM / devfreq: rockchip: add devfreq driver for rk3399 dmc
>>    drm/rockchip: Add dmc notifier in vop driver
> The cover-letter includes the duplicate list of patches.
>
> Also, I want to test the build test. but, When I apply these patches,
> merge conflict happen. Could you give the information about base git repository?
>
> Regards,
> Chanwoo Choi
>
>
>>   drivers/clk/rockchip/Makefile               |   1 +
>>   drivers/clk/rockchip/clk-ddr.c              | 146 +++++++++
>>   drivers/clk/rockchip/clk-pll.c              |   4 +-
>>   drivers/clk/rockchip/clk-rk3399.c           |  19 ++
>>   drivers/clk/rockchip/clk.c                  |  11 +-
>>   drivers/clk/rockchip/clk.h                  |  29 +-
>>   drivers/devfreq/Kconfig                     |   1 +
>>   drivers/devfreq/Makefile                    |   1 +
>>   drivers/devfreq/event/Kconfig               |   7 +
>>   drivers/devfreq/event/Makefile              |   1 +
>>   drivers/devfreq/event/rockchip-dfi.c        | 253 +++++++++++++++
>>   drivers/devfreq/rockchip/Kconfig            |   8 +
>>   drivers/devfreq/rockchip/Makefile           |   1 +
>>   drivers/devfreq/rockchip/rk3399_dmc.c       | 473 ++++++++++++++++++++++++++++
>>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 124 +++++++-
>>   include/dt-bindings/clock/rk3399-cru.h      |   1 +
>>   include/soc/rockchip/rockchip_sip.h         |  27 ++
>>   17 files changed, 1098 insertions(+), 9 deletions(-)
>>   create mode 100644 drivers/clk/rockchip/clk-ddr.c
>>   create mode 100644 drivers/devfreq/event/rockchip-dfi.c
>>   create mode 100644 drivers/devfreq/rockchip/Kconfig
>>   create mode 100644 drivers/devfreq/rockchip/Makefile
>>   create mode 100644 drivers/devfreq/rockchip/rk3399_dmc.c
>>   create mode 100644 include/soc/rockchip/rockchip_sip.h
>>
>
>
>

-- 
Lin Huang



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 0/7] rk3399 support ddr frequency scaling

[Chanwoo Choi]

Hi Lin,

On 2016년 08월 01일 16:46, hl wrote:
> Hi Chanwoo Choi,
> 
>     Ah, i am base on https://chromium.googlesource.com/chromiumos/third_party/kernel/v4.4,
> and forget to rebase on https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git, i will fix it in next version.
> I am sorry about that. And can you help to review the devfreq patch first, if something need to update,
> i will do it together. Thanks.

Sure. I'm now reviewing the devfreq patches.

Regards,
Chanwoo Choi

> 
> On 2016年08月01日 15:39, Chanwoo Choi wrote:
>> Hi Lin,
>>
>> On 2016년 07월 29일 16:56, Lin Huang wrote:
>>> rk3399 platform have dfi controller can monitor ddr load,
>>> and dcf controller to handle ddr register so we can get the
>>> right ddr frequency and make ddr controller happy work(which
>>> will implement in bl31). So we do ddr frequency scaling with
>>> following flow:
>>>
>>>          kernel                                bl31
>>>
>>>     monitor ddr load
>>>         |
>>>         |
>>>     get_target_rate
>>>         |
>>>         |           pass rate to bl31
>>>     clk_set_rate(ddr) --------------------->run dcf flow
>>>         |                                   |
>>>         |                                   |
>>>     wait dcf interrupt<-------------------trigger dcf interrupt
>>>         |
>>>         |
>>>           return
>>>
>>> Lin Huang (6):
>>>    clk: rockchip: add new clock-type for the ddrclk
>>>    clk: rockchip: rk3399: add SCLK_DDRCLK ID for ddrc
>>>    clk: rockchip: rk3399: add ddrc clock support
>>>    PM / devfreq: event: support rockchip dfi controller
>>>    PM / devfreq: rockchip: add devfreq driver for rk3399 dmc
>>>    drm/rockchip: Add dmc notifier in vop driver
>>>
>>>
>>> Heiko Stübner (1):
>>>    clk: rockchip: add clock flag parameter when register pll
>>>
>>> Lin Huang (6):
>>>    clk: rockchip: add new clock-type for the ddrclk
>>>    clk: rockchip: rk3399: add SCLK_DDRCLK ID for ddrc
>>>    clk: rockchip: rk3399: add ddrc clock support
>>>    PM / devfreq: event: support rockchip dfi controller
>>>    PM / devfreq: rockchip: add devfreq driver for rk3399 dmc
>>>    drm/rockchip: Add dmc notifier in vop driver
>> The cover-letter includes the duplicate list of patches.
>>
>> Also, I want to test the build test. but, When I apply these patches,
>> merge conflict happen. Could you give the information about base git repository?
>>
>> Regards,
>> Chanwoo Choi
>>
>>
>>>   drivers/clk/rockchip/Makefile               |   1 +
>>>   drivers/clk/rockchip/clk-ddr.c              | 146 +++++++++
>>>   drivers/clk/rockchip/clk-pll.c              |   4 +-
>>>   drivers/clk/rockchip/clk-rk3399.c           |  19 ++
>>>   drivers/clk/rockchip/clk.c                  |  11 +-
>>>   drivers/clk/rockchip/clk.h                  |  29 +-
>>>   drivers/devfreq/Kconfig                     |   1 +
>>>   drivers/devfreq/Makefile                    |   1 +
>>>   drivers/devfreq/event/Kconfig               |   7 +
>>>   drivers/devfreq/event/Makefile              |   1 +
>>>   drivers/devfreq/event/rockchip-dfi.c        | 253 +++++++++++++++
>>>   drivers/devfreq/rockchip/Kconfig            |   8 +
>>>   drivers/devfreq/rockchip/Makefile           |   1 +
>>>   drivers/devfreq/rockchip/rk3399_dmc.c       | 473 ++++++++++++++++++++++++++++
>>>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 124 +++++++-
>>>   include/dt-bindings/clock/rk3399-cru.h      |   1 +
>>>   include/soc/rockchip/rockchip_sip.h         |  27 ++
>>>   17 files changed, 1098 insertions(+), 9 deletions(-)
>>>   create mode 100644 drivers/clk/rockchip/clk-ddr.c
>>>   create mode 100644 drivers/devfreq/event/rockchip-dfi.c
>>>   create mode 100644 drivers/devfreq/rockchip/Kconfig
>>>   create mode 100644 drivers/devfreq/rockchip/Makefile
>>>   create mode 100644 drivers/devfreq/rockchip/rk3399_dmc.c
>>>   create mode 100644 include/soc/rockchip/rockchip_sip.h
>>>
>>
>>
>>
> 


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v4 0/7] rk3399 support ddr frequency scaling

[Tomeu Vizoso]

On 29 July 2016 at 09:56, Lin Huang <hl@rock-chips.com> wrote:
> rk3399 platform have dfi controller can monitor ddr load,
> and dcf controller to handle ddr register so we can get the
> right ddr frequency and make ddr controller happy work(which
> will implement in bl31). So we do ddr frequency scaling with
> following flow:
>
>              kernel                                bl31
>
>         monitor ddr load
>                 |
>                 |
>         get_target_rate
>                 |
>                 |           pass rate to bl31
>         clk_set_rate(ddr) --------------------->run dcf flow
>                 |                                   |
>                 |                                   |
>         wait dcf interrupt<-------------------trigger dcf interrupt
>                 |
>                 |
>               return

Hi Lin,

can you confirm that the hardware isn't capable of raising interrupts
when programmable load thresholds are trespassed? From reading the
TRM, looks like it should be possible.

Or is there any reason to merge now a driver that polls for load stats?

Thanks,

Tomeu

> Lin Huang (6):
>   clk: rockchip: add new clock-type for the ddrclk
>   clk: rockchip: rk3399: add SCLK_DDRCLK ID for ddrc
>   clk: rockchip: rk3399: add ddrc clock support
>   PM / devfreq: event: support rockchip dfi controller
>   PM / devfreq: rockchip: add devfreq driver for rk3399 dmc
>   drm/rockchip: Add dmc notifier in vop driver
>
>
> Heiko Stübner (1):
>   clk: rockchip: add clock flag parameter when register pll
>
> Lin Huang (6):
>   clk: rockchip: add new clock-type for the ddrclk
>   clk: rockchip: rk3399: add SCLK_DDRCLK ID for ddrc
>   clk: rockchip: rk3399: add ddrc clock support
>   PM / devfreq: event: support rockchip dfi controller
>   PM / devfreq: rockchip: add devfreq driver for rk3399 dmc
>   drm/rockchip: Add dmc notifier in vop driver
>
>  drivers/clk/rockchip/Makefile               |   1 +
>  drivers/clk/rockchip/clk-ddr.c              | 146 +++++++++
>  drivers/clk/rockchip/clk-pll.c              |   4 +-
>  drivers/clk/rockchip/clk-rk3399.c           |  19 ++
>  drivers/clk/rockchip/clk.c                  |  11 +-
>  drivers/clk/rockchip/clk.h                  |  29 +-
>  drivers/devfreq/Kconfig                     |   1 +
>  drivers/devfreq/Makefile                    |   1 +
>  drivers/devfreq/event/Kconfig               |   7 +
>  drivers/devfreq/event/Makefile              |   1 +
>  drivers/devfreq/event/rockchip-dfi.c        | 253 +++++++++++++++
>  drivers/devfreq/rockchip/Kconfig            |   8 +
>  drivers/devfreq/rockchip/Makefile           |   1 +
>  drivers/devfreq/rockchip/rk3399_dmc.c       | 473 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 124 +++++++-
>  include/dt-bindings/clock/rk3399-cru.h      |   1 +
>  include/soc/rockchip/rockchip_sip.h         |  27 ++
>  17 files changed, 1098 insertions(+), 9 deletions(-)
>  create mode 100644 drivers/clk/rockchip/clk-ddr.c
>  create mode 100644 drivers/devfreq/event/rockchip-dfi.c
>  create mode 100644 drivers/devfreq/rockchip/Kconfig
>  create mode 100644 drivers/devfreq/rockchip/Makefile
>  create mode 100644 drivers/devfreq/rockchip/rk3399_dmc.c
>  create mode 100644 include/soc/rockchip/rockchip_sip.h
>
> --
> 1.9.1
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel