[RFC PATCH v1 0/6] rk3399 support ddr frequency scaling

[RFC PATCH v1 0/6] 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):
  rockchip: 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              | 147 ++++++++++++
 drivers/clk/rockchip/clk-rk3399.c           |  20 ++
 drivers/clk/rockchip/clk.c                  |   9 +
 drivers/clk/rockchip/clk.h                  |  27 +++
 drivers/devfreq/Kconfig                     |   1 +
 drivers/devfreq/Makefile                    |   1 +
 drivers/devfreq/event/Kconfig               |   7 +
 drivers/devfreq/event/Makefile              |   1 +
 drivers/devfreq/event/rockchip-dfi.c        | 265 ++++++++++++++++++++++
 drivers/devfreq/rockchip/Kconfig            |  15 ++
 drivers/devfreq/rockchip/Makefile           |   2 +
 drivers/devfreq/rockchip/rk3399_dmc.c       | 337 ++++++++++++++++++++++++++++
 drivers/devfreq/rockchip/rockchip_dmc.c     | 143 ++++++++++++
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  43 +++-
 include/dt-bindings/clock/rk3399-cru.h      |   1 +
 include/soc/rockchip/rockchip_dmc.h         |  45 ++++
 17 files changed, 1063 insertions(+), 2 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 drivers/devfreq/rockchip/rockchip_dmc.c
 create mode 100644 include/soc/rockchip/rockchip_dmc.h

-- 
1.9.1

[RFC PATCH v1 1/6] rockchip: 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 v1:
- None

 drivers/clk/rockchip/Makefile  |   1 +
 drivers/clk/rockchip/clk-ddr.c | 147 +++++++++++++++++++++++++++++++++++++++++
 drivers/clk/rockchip/clk.c     |   9 +++
 drivers/clk/rockchip/clk.h     |  27 ++++++++
 4 files changed, 184 insertions(+)
 create mode 100644 drivers/clk/rockchip/clk-ddr.c

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..5b6630d
--- /dev/null
+++ b/drivers/clk/rockchip/clk-ddr.c
@@ -0,0 +1,147 @@
+/*
+ * 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/of.h>
+#include <linux/slab.h>
+#include <linux/io.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.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)
+#define val_mask(width)	((1 << (width)) - 1)
+
+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;
+
+	spin_lock_irqsave(ddrclk->lock, flags);
+
+	/* TODO: set ddr rate in bl31 */
+
+	spin_unlock_irqrestore(ddrclk->lock, flags);
+
+	return 0;
+}
+
+static unsigned long
+rockchip_ddrclk_recalc_rate(struct clk_hw *hw,
+			    unsigned long parent_rate)
+{
+	struct rockchip_ddrclk *ddrclk = to_rockchip_ddrclk_hw(hw);
+	int val;
+
+	val = clk_readl(ddrclk->reg_base +
+			ddrclk->mux_offset) >> ddrclk->div_shift;
+	val &= val_mask(ddrclk->div_width);
+
+	return DIV_ROUND_UP_ULL((u64)parent_rate, val + 1);
+}
+
+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 &= val_mask(ddrclk->mux_width);
+
+	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;
+	int ret;
+
+	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);
+		ret = PTR_ERR(clk);
+		goto free_ddrclk;
+	}
+
+	return clk;
+
+free_ddrclk:
+	kfree(ddrclk);
+	return ERR_PTR(ret);
+}
diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c
index 7ffd134..6ac1aa5 100644
--- a/drivers/clk/rockchip/clk.c
+++ b/drivers/clk/rockchip/clk.c
@@ -484,6 +484,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 2194ffa..4033fe4 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,				\
-- 
1.9.1

[RFC PATCH v1 2/6] clk: rockchip: rk3399: add SCLK_DDRCLK ID for ddrc

[Lin Huang]

Signed-off-by: Lin Huang <hl@rock-chips.com>
---
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

[RFC PATCH v1 3/6] 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 v1:
- remove ddrc source CLK_IGNORE_UNUSED flag, Suggestion by Doug
- move clk_ddrc and clk_ddrc_dpll_src to critical, Suggestion by Doug

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

diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c
index f1d8e44..29afb88 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,10 @@ static const char *const rk3399_cru_critical_clocks[] __initconst = {
 	"gpll_hclk_perilp1_src",
 	"gpll_aclk_perilp0_src",
 	"gpll_aclk_perihp_src",
+
+	/* ddrc */
+	"clk_ddrc_dpll_src",
+	"clk_ddrc",
 };
 
 static const char *const rk3399_pmucru_critical_clocks[] __initconst = {
-- 
1.9.1

[RFC PATCH v1 4/6] 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>
---
Changes in v1:
- NOne

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

diff --git a/drivers/devfreq/event/Kconfig b/drivers/devfreq/event/Kconfig
index 1e8b4f4..6ebdc13 100644
--- a/drivers/devfreq/event/Kconfig
+++ b/drivers/devfreq/event/Kconfig
@@ -30,4 +30,11 @@ config DEVFREQ_EVENT_EXYNOS_PPMU
 	  (Platform Performance Monitoring Unit) counters to estimate the
 	  utilization of each module.
 
+config DEVFREQ_EVENT_ROCKCHIP_DFI
+	bool "ROCKCHIP DFI DEVFREQ event Driver"
+	depends on ARCH_ROCKCHIP
+	help
+	  This add the devfreq-event driver for Rockchip SoC. It provides DFI
+	  driver to count ddr load.
+
 endif # PM_DEVFREQ_EVENT
diff --git a/drivers/devfreq/event/Makefile b/drivers/devfreq/event/Makefile
index 3d6afd3..dda7090 100644
--- a/drivers/devfreq/event/Makefile
+++ b/drivers/devfreq/event/Makefile
@@ -2,3 +2,4 @@
 
 obj-$(CONFIG_DEVFREQ_EVENT_EXYNOS_NOCP) += exynos-nocp.o
 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..e3b020f9
--- /dev/null
+++ b/drivers/devfreq/event/rockchip-dfi.c
@@ -0,0 +1,265 @@
+/*
+ * 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(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_enable(info->clk);
+	if (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;
+	int ret;
+	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)) {
+		ret = PTR_ERR(data->edev);
+		dev_err(&pdev->dev,
+			"failed to add devfreq-event device\n");
+		return ret;
+	}
+
+	ret = clk_prepare_enable(data->clk);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to enable clk: %d\n", ret);
+		clk_disable_unprepare(data->clk);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, data);
+
+	return 0;
+}
+
+static int rockchip_dfi_remove(struct platform_device *pdev)
+{
+	return 0;
+}
+
+static struct platform_driver rockchip_dfi_driver = {
+	.probe	= rockchip_dfi_probe,
+	.remove	= rockchip_dfi_remove,
+	.driver = {
+		.name	= "rockchip-dfi",
+		.of_match_table = rockchip_dfi_id_match,
+	},
+};
+module_platform_driver(rockchip_dfi_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Rockchip dfi driver");
-- 
1.9.1

[RFC PATCH v1 5/6] 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 v1:
- move dfi controller to event, Suggestion by Chanwoo Choi
- fix set voltage sequence when set rate fail
- change Kconfig type from tristate to bool, Suggestion by MyungJoo Ham
- move unuse EXPORT_SYMBOL_GPL(), Suggestion by MyungJoo Ham

 drivers/devfreq/Kconfig                 |   1 +
 drivers/devfreq/Makefile                |   1 +
 drivers/devfreq/rockchip/Kconfig        |  15 ++
 drivers/devfreq/rockchip/Makefile       |   2 +
 drivers/devfreq/rockchip/rk3399_dmc.c   | 337 ++++++++++++++++++++++++++++++++
 drivers/devfreq/rockchip/rockchip_dmc.c | 143 ++++++++++++++
 include/soc/rockchip/rockchip_dmc.h     |  45 +++++
 7 files changed, 544 insertions(+)
 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 drivers/devfreq/rockchip/rockchip_dmc.c
 create mode 100644 include/soc/rockchip/rockchip_dmc.h

diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index 78dac0e..e57275e 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -101,5 +101,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 09f11d9..48e2ae6 100644
--- a/drivers/devfreq/Makefile
+++ b/drivers/devfreq/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE)	+= governor_passive.o
 # DEVFREQ Drivers
 obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ)	+= exynos-bus.o
 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..7fb1cff
--- /dev/null
+++ b/drivers/devfreq/rockchip/Kconfig
@@ -0,0 +1,15 @@
+config ARM_ROCKCHIP_DMC_DEVFREQ
+	bool "ARM ROCKCHIP DMC DEVFREQ Driver"
+	depends on ARCH_ROCKCHIP
+	help
+	  This adds the DEVFREQ driver framework for the rockchip dmc.
+
+config ARM_RK3399_DMC_DEVFREQ
+	bool "ARM RK3399 DMC DEVFREQ Driver"
+	depends on ARM_ROCKCHIP_DMC_DEVFREQ
+	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..caca525
--- /dev/null
+++ b/drivers/devfreq/rockchip/Makefile
@@ -0,0 +1,2 @@
+obj-$(CONFIG_ARM_ROCKCHIP_DMC_DEVFREQ)	+= rockchip_dmc.o
+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..41ff71b
--- /dev/null
+++ b/drivers/devfreq/rockchip/rk3399_dmc.c
@@ -0,0 +1,337 @@
+/*
+ * 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/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_dmc.h>
+
+struct rk3399_dmcfreq {
+	struct device *dev;
+	struct devfreq *devfreq;
+	struct devfreq_simple_ondemand_data ondemand_data;
+	struct clk *dmc_clk;
+	struct completion dcf_hold_completion;
+	struct devfreq_event_dev *edev;
+	struct mutex lock;
+	struct notifier_block dmc_nb;
+	int irq;
+	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;
+		}
+	}
+
+	dmc_event(DMCFREQ_ADJUST);
+	reinit_completion(&dmcfreq->dcf_hold_completion);
+	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);
+		dmc_event(DMCFREQ_FINISH);
+		goto out;
+	}
+
+	/* wait until bcf irq happen, it means freq scaling finish in bl31 */
+	wait_for_completion(&dmcfreq->dcf_hold_completion);
+	dmc_event(DMCFREQ_FINISH);
+
+	/*
+	 * check the rate we get whether is correct
+	 * in bl31 dcf, 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);
+
+	/* do not get the right 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)
+{
+	rockchip_dmc_disable();
+	return 0;
+}
+
+static __maybe_unused int rk3399_dmcfreq_resume(struct device *dev)
+{
+	rockchip_dmc_enable();
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(rk3399_dmcfreq_pm, rk3399_dmcfreq_suspend,
+			 rk3399_dmcfreq_resume);
+
+static int rk3399_dmc_enable_notify(struct notifier_block *nb,
+				    unsigned long event, void *data)
+{
+	struct rk3399_dmcfreq *dmcfreq =
+			      container_of(nb, struct rk3399_dmcfreq, dmc_nb);
+	unsigned long freq = ULONG_MAX;
+
+	if (event == DMC_ENABLE) {
+		devfreq_event_enable_edev(dmcfreq->edev);
+		devfreq_resume_device(dmcfreq->devfreq);
+		return NOTIFY_OK;
+	} else if (event == DMC_DISABLE) {
+		devfreq_event_disable_edev(dmcfreq->edev);
+		devfreq_suspend_device(dmcfreq->devfreq);
+
+		/* when disable dmc, set sdram to max frequency */
+		rk3399_dmcfreq_target(dmcfreq->dev, &freq, 0);
+		return NOTIFY_OK;
+	}
+
+	return NOTIFY_DONE;
+}
+
+static irqreturn_t rk3399_dmc_irq(int irq, void *dev_id)
+{
+	struct rk3399_dmcfreq *dmcfreq = dev_id;
+
+	complete(&dmcfreq->dcf_hold_completion);
+
+	return IRQ_HANDLED;
+}
+
+static int rk3399_dmcfreq_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rk3399_dmcfreq *data;
+	int ret, irq;
+	struct device_node *np = pdev->dev.of_node;
+
+	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->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->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->dmc_nb.notifier_call = rk3399_dmc_enable_notify;
+	dmc_register_notifier(&data->dmc_nb);
+
+	init_completion(&data->dcf_hold_completion);
+
+	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;
+	}
+
+	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");
diff --git a/drivers/devfreq/rockchip/rockchip_dmc.c b/drivers/devfreq/rockchip/rockchip_dmc.c
new file mode 100644
index 0000000..78baee5
--- /dev/null
+++ b/drivers/devfreq/rockchip/rockchip_dmc.c
@@ -0,0 +1,143 @@
+/*
+ * Copyright (c) 2016, Fuzhou Rockchip Electronics Co., Ltd
+ * Author: Lin Huang <hl@rock-chips.com>
+ * Base on: https://chromium-review.googlesource.com/#/c/231477/
+ *
+ * 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/mutex.h>
+#include <soc/rockchip/rockchip_dmc.h>
+
+static int num_wait;
+static int num_disable;
+static BLOCKING_NOTIFIER_HEAD(dmc_notifier_list);
+static DEFINE_MUTEX(dmc_en_lock);
+static DEFINE_MUTEX(dmc_sync_lock);
+
+/**
+ * rockchip_dmc_enabled - Returns true if dmc freq is enabled, false otherwise.
+ */
+bool rockchip_dmc_enabled(void)
+{
+	return num_disable <= 0 && num_wait <= 1;
+}
+
+/**
+ * rockchip_dmc_enable - Enable dmc frequency scaling. Will only enable
+ * frequency scaling if there are 1 or fewer notifiers. Call to undo
+ * rockchip_dmc_disable.
+ */
+void rockchip_dmc_enable(void)
+{
+	mutex_lock(&dmc_en_lock);
+	num_disable--;
+	WARN_ON(num_disable < 0);
+	if (rockchip_dmc_enabled())
+		dmc_event(DMC_ENABLE);
+	mutex_unlock(&dmc_en_lock);
+}
+
+/**
+ * rockchip_dmc_disable - Disable dmc frequency scaling. Call when something
+ * cannot coincide with dmc frequency scaling.
+ */
+void rockchip_dmc_disable(void)
+{
+	mutex_lock(&dmc_en_lock);
+	if (rockchip_dmc_enabled())
+		dmc_event(DMC_DISABLE);
+	num_disable++;
+	mutex_unlock(&dmc_en_lock);
+}
+
+void dmc_event(int event)
+{
+	mutex_lock(&dmc_sync_lock);
+	blocking_notifier_call_chain(&dmc_notifier_list, event, NULL);
+	mutex_unlock(&dmc_sync_lock);
+}
+
+/**
+ * dmc_register_notifier - register a driver to dmc chain
+ * @nb: notifier function to register
+ */
+int dmc_register_notifier(struct notifier_block *nb)
+{
+	int ret;
+
+	if (!nb)
+		return -EINVAL;
+
+	ret = blocking_notifier_chain_register(&dmc_notifier_list, nb);
+
+	return ret;
+}
+
+/**
+ * dmc_unregister_notifier - unregister a driver from dmc chain
+ * @nb: remove notifier function
+ */
+int dmc_unregister_notifier(struct notifier_block *nb)
+{
+	int ret;
+
+	if (!nb)
+		return -EINVAL;
+
+	ret = blocking_notifier_chain_unregister(&dmc_notifier_list, nb);
+
+	return ret;
+}
+
+/**
+ * rockchip_dmc_get - Register the notifier block for the dmc chain.
+ * @nb The dmc notifier block to register
+ */
+int rockchip_dmc_get(struct notifier_block *nb)
+{
+	if (!nb)
+		return -EINVAL;
+
+	mutex_lock(&dmc_en_lock);
+
+	/*
+	 * if have two notifier(enable two vop etc),
+	 * need to disable dmc
+	 */
+	if (num_wait == 1 && num_disable <= 0)
+		dmc_event(DMC_DISABLE);
+	num_wait++;
+	dmc_register_notifier(nb);
+	mutex_unlock(&dmc_en_lock);
+
+	return 0;
+}
+
+/**
+ * rockchip_dmc_put - Remove the notifier block from the dmc chain.
+ * @nb The dmc notifier block to unregister
+ */
+int rockchip_dmc_put(struct notifier_block *nb)
+{
+	if (!nb)
+		return -EINVAL;
+
+	mutex_lock(&dmc_en_lock);
+	num_wait--;
+
+	/* if notifier from 2 back to 1, enable dmc again */
+	if (num_wait == 1 && num_disable <= 0)
+		dmc_event(DMC_ENABLE);
+	dmc_unregister_notifier(nb);
+	mutex_unlock(&dmc_en_lock);
+
+	return 0;
+}
diff --git a/include/soc/rockchip/rockchip_dmc.h b/include/soc/rockchip/rockchip_dmc.h
new file mode 100644
index 0000000..3f69cbf
--- /dev/null
+++ b/include/soc/rockchip/rockchip_dmc.h
@@ -0,0 +1,45 @@
+/*
+ * Copyright (c) 2016, Fuzhou Rockchip Electronics Co., Ltd
+ *
+ * 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_DMC_H
+#define __SOC_ROCKCHIP_DMC_H
+
+#include <linux/notifier.h>
+
+#define DMC_ENABLE	0
+#define DMC_DISABLE	1
+#define DMCFREQ_ADJUST	2
+#define DMCFREQ_FINISH	3
+
+#if IS_ENABLED(CONFIG_ARM_ROCKCHIP_DMC_DEVFREQ)
+int rockchip_dmc_get(struct notifier_block *nb);
+int rockchip_dmc_put(struct notifier_block *nb);
+#else
+static inline int rockchip_dmc_get(struct notifier_block *nb)
+{
+	return 0;
+}
+
+static inline int rockchip_dmc_put(struct notifier_block *nb)
+{
+	return 0;
+}
+#endif
+
+void dmc_event(int event);
+int dmc_register_notifier(struct notifier_block *nb);
+int dmc_unregister_notifier(struct notifier_block *nb);
+void rockchip_dmc_enable(void);
+void rockchip_dmc_disable(void);
+bool rockchip_dmc_enabled(void);
+#endif
-- 
1.9.1

[RFC PATCH v1 6/6] drm/rockchip: Add dmc notifier in vop driver

[Lin Huang]

when in ddr frequency scaling process, vop can not do
enable or disable operate, 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 dmc notifier, and we can
get the dmc status.

Signed-off-by: Lin Huang <hl@rock-chips.com>
---
Changes in v1:
- use wait_event instead usleep, Suggestion by Derek

 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 43 +++++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 1c4d5b5..8286048 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -31,6 +31,8 @@
 #include <linux/reset.h>
 #include <linux/delay.h>
 
+#include <soc/rockchip/rockchip_dmc.h>
+
 #include "rockchip_drm_drv.h"
 #include "rockchip_drm_gem.h"
 #include "rockchip_drm_fb.h"
@@ -116,6 +118,10 @@ struct vop {
 
 	const struct vop_data *data;
 
+	struct notifier_block dmc_nb;
+	int dmc_in_process;
+	wait_queue_head_t	wait_dmc_queue;
+
 	uint32_t *regsbak;
 	void __iomem *regs;
 
@@ -426,6 +432,21 @@ static void vop_dsp_hold_valid_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 == DMCFREQ_ADJUST) {
+		vop->dmc_in_process = 1;
+	} else if (event == DMCFREQ_FINISH) {
+		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);
@@ -434,6 +455,13 @@ static void vop_enable(struct drm_crtc *crtc)
 	if (vop->is_enabled)
 		return;
 
+	/*
+	 * if in dmc scaling frequency process, wait until it finish
+	 * use 100ms as timeout time.
+	 */
+	wait_event_timeout(vop->wait_dmc_queue,
+			   !vop->dmc_in_process, HZ / 10);
+
 	ret = pm_runtime_get_sync(vop->dev);
 	if (ret < 0) {
 		dev_err(vop->dev, "failed to get pm runtime: %d\n", ret);
@@ -485,6 +513,7 @@ static void vop_enable(struct drm_crtc *crtc)
 	enable_irq(vop->irq);
 
 	drm_crtc_vblank_on(crtc);
+	rockchip_dmc_get(&vop->dmc_nb);
 
 	return;
 
@@ -505,6 +534,13 @@ static void vop_crtc_disable(struct drm_crtc *crtc)
 		return;
 
 	/*
+	 * if in dmc scaling frequency process, wait until it finish
+	 * use 100ms as timeout time.
+	 */
+	wait_event_timeout(vop->wait_dmc_queue,
+			   !vop->dmc_in_process, HZ / 10);
+
+	/*
 	 * 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.
@@ -517,7 +553,7 @@ static void vop_crtc_disable(struct drm_crtc *crtc)
 		VOP_WIN_SET(vop, win, enable, 0);
 		spin_unlock(&vop->reg_lock);
 	}
-
+	rockchip_dmc_put(&vop->dmc_nb);
 	drm_crtc_vblank_off(crtc);
 
 	/*
@@ -1243,7 +1279,7 @@ static int vop_create_crtc(struct vop *vop)
 		ret = -ENOENT;
 		goto err_cleanup_crtc;
 	}
-
+	vop->dmc_nb.notifier_call = dmc_notify;
 	init_completion(&vop->dsp_hold_completion);
 	init_completion(&vop->wait_update_complete);
 	crtc->port = port;
@@ -1465,6 +1501,9 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
 	/* IRQ is initially disabled; it gets enabled in power_on */
 	disable_irq(vop->irq);
 
+	init_waitqueue_head(&vop->wait_dmc_queue);
+	vop->dmc_in_process = 0;
+
 	ret = vop_create_crtc(vop);
 	if (ret)
 		return ret;
-- 
1.9.1

Re: [RFC PATCH v1 4/6] PM / devfreq: event: support rockchip dfi controller

[Chanwoo Choi]

Hi Lin,

I add the some comment on below. If you modify it,
You can add my acked-by tag. Looks good to me.

Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

Also, I'd like you to add me to mail thread
on next version because I'm supporter of devfreq-event.

On 2016년 06월 03일 18:55, 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>
> ---
> Changes in v1:
> - NOne
> 
>  drivers/devfreq/event/Kconfig        |   7 +
>  drivers/devfreq/event/Makefile       |   1 +
>  drivers/devfreq/event/rockchip-dfi.c | 265 +++++++++++++++++++++++++++++++++++
>  3 files changed, 273 insertions(+)
>  create mode 100644 drivers/devfreq/event/rockchip-dfi.c
> 
> diff --git a/drivers/devfreq/event/Kconfig b/drivers/devfreq/event/Kconfig
> index 1e8b4f4..6ebdc13 100644
> --- a/drivers/devfreq/event/Kconfig
> +++ b/drivers/devfreq/event/Kconfig
> @@ -30,4 +30,11 @@ config DEVFREQ_EVENT_EXYNOS_PPMU
>  	  (Platform Performance Monitoring Unit) counters to estimate the
>  	  utilization of each module.
>  
> +config DEVFREQ_EVENT_ROCKCHIP_DFI
> +	bool "ROCKCHIP DFI DEVFREQ event Driver"
> +	depends on ARCH_ROCKCHIP
> +	help
> +	  This add the devfreq-event driver for Rockchip SoC. It provides DFI

You better to full name of 'DFI' abbreviation as following:
- DFI (Dxxx Fxxx Ixxx)

> +	  driver to count ddr load.
> +
>  endif # PM_DEVFREQ_EVENT
> diff --git a/drivers/devfreq/event/Makefile b/drivers/devfreq/event/Makefile
> index 3d6afd3..dda7090 100644
> --- a/drivers/devfreq/event/Makefile
> +++ b/drivers/devfreq/event/Makefile
> @@ -2,3 +2,4 @@
>  
>  obj-$(CONFIG_DEVFREQ_EVENT_EXYNOS_NOCP) += exynos-nocp.o
>  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..e3b020f9
> --- /dev/null
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -0,0 +1,265 @@
> +/*
> + * 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(info->clk);

I prefer to change the function as following. I add the comment on below in probe()
- clk_disable -> clk_disable_unprepare()


> +
> +	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_enable(info->clk);

I prefer to change the function as following. I add the comment on below in probe()
- clk_enable -> clk_prepare_enable()

> +	if (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;
> +	int ret;
> +	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)) {
> +		ret = PTR_ERR(data->edev);
> +		dev_err(&pdev->dev,
> +			"failed to add devfreq-event device\n");
> +		return ret;

You can simply return the PTR_ERR(data->edev) without 'ret' variable as following:

	return PTR_ERR(data->edev);


> +	}
> +
> +	ret = clk_prepare_enable(data->clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to enable clk: %d\n", ret);
> +		clk_disable_unprepare(data->clk);
> +		return ret;
> +	}

The following two functions handle the clock. So, rockchip_dfi_probe()
don't need to enable the clock. Just pass the role of clock control to the following functions.
Because of calling the twice of enable function of clock, the usage count of clock
is mismatch when disabling the clock.
- rockchip_dfi_enable(struct devfreq_event_dev *edev) enable the clock.
- rockchip_dfi_disable(struct devfreq_event_dev *edev) disable the clock.


> +
> +	platform_set_drvdata(pdev, data);
> +
> +	return 0;
> +}
> +
> +static int rockchip_dfi_remove(struct platform_device *pdev)
> +{
> +	return 0;
> +}

If the rockchip_dfi_remove() don't do anything, you can remove it

> +
> +static struct platform_driver rockchip_dfi_driver = {
> +	.probe	= rockchip_dfi_probe,
> +	.remove	= rockchip_dfi_remove,

ditto.

> +	.driver = {
> +		.name	= "rockchip-dfi",
> +		.of_match_table = rockchip_dfi_id_match,
> +	},
> +};
> +module_platform_driver(rockchip_dfi_driver);
> +
> +MODULE_LICENSE("GPL v2");

You need to add MODULE_AUTHOR("").

> +MODULE_DESCRIPTION("Rockchip dfi driver");
> 

Remove unneeded blank line

Regards,
Chanwoo Choi

Re: [RFC PATCH v1 1/6] rockchip: rockchip: add new clock-type for the ddrclk

[Shawn Lin]

Hi Lin,

It looks good with only a few minor comments.

On 2016/6/3 17:55, Lin Huang wrote:
> 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 v1:
> - None
>
>  drivers/clk/rockchip/Makefile  |   1 +
>  drivers/clk/rockchip/clk-ddr.c | 147 +++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/rockchip/clk.c     |   9 +++
>  drivers/clk/rockchip/clk.h     |  27 ++++++++
>  4 files changed, 184 insertions(+)
>  create mode 100644 drivers/clk/rockchip/clk-ddr.c
>
> 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..5b6630d
> --- /dev/null
> +++ b/drivers/clk/rockchip/clk-ddr.c
> @@ -0,0 +1,147 @@
> +/*
> + * 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/of.h>
> +#include <linux/slab.h>
> +#include <linux/io.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include "clk.h"
> +

alphabetical order would be better.

> +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)
> +#define val_mask(width)	((1 << (width)) - 1)
> +
> +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;
> +
> +	spin_lock_irqsave(ddrclk->lock, flags);
> +
> +	/* TODO: set ddr rate in bl31 */
> +
> +	spin_unlock_irqrestore(ddrclk->lock, flags);
> +

What do you wanna protect here?

> +	return 0;
> +}
> +
> +static unsigned long
> +rockchip_ddrclk_recalc_rate(struct clk_hw *hw,
> +			    unsigned long parent_rate)
> +{
> +	struct rockchip_ddrclk *ddrclk = to_rockchip_ddrclk_hw(hw);
> +	int val;
> +
> +	val = clk_readl(ddrclk->reg_base +
> +			ddrclk->mux_offset) >> ddrclk->div_shift;
> +	val &= val_mask(ddrclk->div_width);
> +
> +	return DIV_ROUND_UP_ULL((u64)parent_rate, val + 1);
> +}
> +
> +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 &= val_mask(ddrclk->mux_width);
> +
> +	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;
> +	int ret;
> +
> +	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;

can we combine these two?

> +
> +	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);
> +		ret = PTR_ERR(clk);

Why do you need PTR_ERR and then convert back again.

> +		goto free_ddrclk;
> +	}
> +
> +	return clk;
> +
> +free_ddrclk:
> +	kfree(ddrclk);
> +	return ERR_PTR(ret);
> +}
> diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c
> index 7ffd134..6ac1aa5 100644
> --- a/drivers/clk/rockchip/clk.c
> +++ b/drivers/clk/rockchip/clk.c
> @@ -484,6 +484,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 2194ffa..4033fe4 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)				\

it seems a duplication exept for branch_type.

> +	{							\
> +		.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,				\
>


-- 
Best Regards
Shawn Lin

Re: [RFC PATCH v1 2/6] clk: rockchip: rk3399: add SCLK_DDRCLK ID for ddrc

[Shawn Lin]

Hi Lin

How about merge it into your patch#3.

On 2016/6/3 17:55, Lin Huang wrote:
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> ---
> 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
>


-- 
Best Regards
Shawn Lin

Re: [RFC PATCH v1 2/6] clk: rockchip: rk3399: add SCLK_DDRCLK ID for ddrc

[Heiko Stübner]

Hi Shawn,

Am Freitag, 3. Juni 2016, 20:34:52 schrieb Shawn Lin:
> How about merge it into your patch#3.

see comments from Doug and me on previous version.

clock-ids should always be separate patches, as we will need them in both 
clock and devicetree branches, so they must be in a separate branch shared 
between clock and dts branches.


Heiko

> On 2016/6/3 17:55, Lin Huang wrote:
> > Signed-off-by: Lin Huang <hl@rock-chips.com>
> > ---
> > 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

Re: [RFC PATCH v1 2/6] clk: rockchip: rk3399: add SCLK_DDRCLK ID for ddrc

[Shawn Lin]

在 2016/6/3 20:36, Heiko Stübner 写道:
> Hi Shawn,
>
> Am Freitag, 3. Juni 2016, 20:34:52 schrieb Shawn Lin:
>> How about merge it into your patch#3.
>
> see comments from Doug and me on previous version.
>
> clock-ids should always be separate patches, as we will need them in both
> clock and devicetree branches, so they must be in a separate branch shared
> between clock and dts branches.
>

Ah, I missed the previous version as I think it should be from
non-version to v2 rather than from non-version to v1, which IIRC is from
Doug. :)

Anyway, thanks for explaining this.

>
> Heiko
>
>> On 2016/6/3 17:55, Lin Huang wrote:
>>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>>> ---
>>> 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
>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>


-- 
Best Regards
Shawn Lin

Re: [RFC PATCH v1 1/6] rockchip: rockchip: add new clock-type for the ddrclk

[Heiko Stübner]

Am Freitag, 3. Juni 2016, 17:55:14 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>
> ---
> Changes in v1:
> - None
> 
>  drivers/clk/rockchip/Makefile  |   1 +
>  drivers/clk/rockchip/clk-ddr.c | 147
> +++++++++++++++++++++++++++++++++++++++++ drivers/clk/rockchip/clk.c     | 
>  9 +++
>  drivers/clk/rockchip/clk.h     |  27 ++++++++
>  4 files changed, 184 insertions(+)
>  create mode 100644 drivers/clk/rockchip/clk-ddr.c
> 
> 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..5b6630d
> --- /dev/null
> +++ b/drivers/clk/rockchip/clk-ddr.c
> @@ -0,0 +1,147 @@
> +/*
> + * 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/of.h>
> +#include <linux/slab.h>
> +#include <linux/io.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.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)
> +#define val_mask(width)	((1 << (width)) - 1)

maybe use GENMASK?

> +
> +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;
> +
> +	spin_lock_irqsave(ddrclk->lock, flags);
> +
> +	/* TODO: set ddr rate in bl31 */

I expect this interface to be in existence and merged into the main ATF first.

Right now the whole clock-type does nothing more than a simple COMPOSITE with 
added CLK_DIVIDER_READ_ONLY | CLK_MUX_READ_ONLY.

Also Mike is propably still working in the so called coordinated rate change 
for clocks needing special handling on rate changes, which might provide a 
second approach to this.

So please, first of all get the ATF-interface merged and meanwhile if you need 
to read the clock-rate, just use a regular composite, with the read-only flags.


> +	spin_unlock_irqrestore(ddrclk->lock, flags);
> +
> +	return 0;
> +}
> +
> +static unsigned long
> +rockchip_ddrclk_recalc_rate(struct clk_hw *hw,
> +			    unsigned long parent_rate)
> +{
> +	struct rockchip_ddrclk *ddrclk = to_rockchip_ddrclk_hw(hw);
> +	int val;
> +
> +	val = clk_readl(ddrclk->reg_base +
> +			ddrclk->mux_offset) >> ddrclk->div_shift;
> +	val &= val_mask(ddrclk->div_width);
> +
> +	return DIV_ROUND_UP_ULL((u64)parent_rate, val + 1);

return divider_recalc_rate(...)


Heiko

Re: [RFC PATCH v1 3/6] clk: rockchip: rk3399: add ddrc clock support

[Heiko Stübner]

Am Freitag, 3. Juni 2016, 17:55:16 schrieb 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 v1:
> - remove ddrc source CLK_IGNORE_UNUSED flag, Suggestion by Doug
> - move clk_ddrc and clk_ddrc_dpll_src to critical, Suggestion by Doug
> 
>  drivers/clk/rockchip/clk-rk3399.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/clk/rockchip/clk-rk3399.c
> b/drivers/clk/rockchip/clk-rk3399.c index f1d8e44..29afb88 100644
> --- a/drivers/clk/rockchip/clk-rk3399.c
> +++ b/drivers/clk/rockchip/clk-rk3399.c

[...]

> @@ -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),
>  };

as said in the other patch, just make this a regular COMPOSITE_NOGATE with 
CLK_DIVIDER_READ_ONLY | CLK_MUX_READ_ONLY until that interface to the ATF 
exists and is approved.

That way you can still read back the clock rate without anything changing the 
clock-rate, but we don't need to add duplicate code for it.


> 
>  static struct rockchip_clk_branch rk3399_clk_pmu_branches[] __initdata = {
> @@ -1487,6 +1503,10 @@ static const char *const rk3399_cru_critical_clocks[]
> __initconst = { "gpll_hclk_perilp1_src",
>  	"gpll_aclk_perilp0_src",
>  	"gpll_aclk_perihp_src",
> +
> +	/* ddrc */
> +	"clk_ddrc_dpll_src",

Why does your clk_ddrc_dpll_src need a separate critical entry. Any code 
changing the clk_ddrc parent should make sure the new parent is enabled. (The 
clock-framework of course does this already).


> +	"clk_ddrc",
>  };
> 
>  static const char *const rk3399_pmucru_critical_clocks[] __initconst = {


Heiko

Re: [RFC PATCH v1 2/6] clk: rockchip: rk3399: add SCLK_DDRCLK ID for ddrc

[Doug Anderson]

Hi,

On Fri, Jun 3, 2016 at 5:47 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> Ah, I missed the previous version as I think it should be from
> non-version to v2 rather than from non-version to v1, which IIRC is from
> Doug. :)

Yup, patch-numbering is 1-based, not 0-based, so typically you have
no-version => v2 => v3 => v99

-Doug

Re: [RFC PATCH v1 4/6] PM / devfreq: event: support rockchip dfi controller

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 529 bytes --]

On Fri, Jun 03, 2016 at 05:55:17PM +0800, Lin Huang wrote:
[...]
> +	ret = clk_prepare_enable(data->clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to enable clk: %d\n", ret);
> +		clk_disable_unprepare(data->clk);
> +		return ret;
> +	}

This is going to give you a large WARN. clk_prepare_enable() already
leaves the clock in a proper state when it fails (i.e. it calls
clk_unprepare() if the clk_enable() part failed), so calling
clk_disable_unprepare() upon failure is going to unbalance the
reference counts.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC PATCH v1 1/6] rockchip: rockchip: add new clock-type for the ddrclk

[hl]

Hi Shawn,

On 2016年06月03日 20:29, Shawn Lin wrote:
> Hi Lin,
>
> It looks good with only a few minor comments.
>
> On 2016/6/3 17:55, Lin Huang wrote:
>> 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 v1:
>> - None
>>
>>  drivers/clk/rockchip/Makefile  |   1 +
>>  drivers/clk/rockchip/clk-ddr.c | 147 
>> +++++++++++++++++++++++++++++++++++++++++
>>  drivers/clk/rockchip/clk.c     |   9 +++
>>  drivers/clk/rockchip/clk.h     |  27 ++++++++
>>  4 files changed, 184 insertions(+)
>>  create mode 100644 drivers/clk/rockchip/clk-ddr.c
>>
>> 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..5b6630d
>> --- /dev/null
>> +++ b/drivers/clk/rockchip/clk-ddr.c
>> @@ -0,0 +1,147 @@
>> +/*
>> + * 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/of.h>
>> +#include <linux/slab.h>
>> +#include <linux/io.h>
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include "clk.h"
>> +
>
> alphabetical order would be better.
     Okay, will fix next version, thank you.
>
>> +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)
>> +#define val_mask(width)    ((1 << (width)) - 1)
>> +
>> +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;
>> +
>> +    spin_lock_irqsave(ddrclk->lock, flags);
>> +
>> +    /* TODO: set ddr rate in bl31 */
>> +
>> +    spin_unlock_irqrestore(ddrclk->lock, flags);
>> +
>
> What do you wanna protect here?
     I am not sure for now, when i finish this function,
     if there nothing need protect i will remove it. Thank you.
>
>> +    return 0;
>> +}
>> +
>> +static unsigned long
>> +rockchip_ddrclk_recalc_rate(struct clk_hw *hw,
>> +                unsigned long parent_rate)
>> +{
>> +    struct rockchip_ddrclk *ddrclk = to_rockchip_ddrclk_hw(hw);
>> +    int val;
>> +
>> +    val = clk_readl(ddrclk->reg_base +
>> +            ddrclk->mux_offset) >> ddrclk->div_shift;
>> +    val &= val_mask(ddrclk->div_width);
>> +
>> +    return DIV_ROUND_UP_ULL((u64)parent_rate, val + 1);
>> +}
>> +
>> +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 &= val_mask(ddrclk->mux_width);
>> +
>> +    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;
>> +    int ret;
>> +
>> +    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;
>
> can we combine these two?
>
>> +
>> +    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);
>> +        ret = PTR_ERR(clk);
>
> Why do you need PTR_ERR and then convert back again.
     Yes, there is only one error for ret value, so i can return "NULL" 
directly, thank you.
>
>> +        goto free_ddrclk;
>> +    }
>> +
>> +    return clk;
>> +
>> +free_ddrclk:
>> +    kfree(ddrclk);
>> +    return ERR_PTR(ret);
>> +}
>> diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c
>> index 7ffd134..6ac1aa5 100644
>> --- a/drivers/clk/rockchip/clk.c
>> +++ b/drivers/clk/rockchip/clk.c
>> @@ -484,6 +484,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 2194ffa..4033fe4 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)                \
>
> it seems a duplication exept for branch_type.
     We just need this different branch_type, so we can add case in
     rockchip_clk_register_branches() and call 
rockchip_clk_register_ddrclk().
>
>> +    {                            \
>> +        .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,                \
>>
>
>

-- 
Lin Huang

Re: [RFC PATCH v1 1/6] rockchip: rockchip: add new clock-type for the ddrclk

[hl]

Hi Heiko,

On 2016年06月03日 20:51, Heiko Stübner wrote:
> Am Freitag, 3. Juni 2016, 17:55:14 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>
>> ---
>> Changes in v1:
>> - None
>>
>>   drivers/clk/rockchip/Makefile  |   1 +
>>   drivers/clk/rockchip/clk-ddr.c | 147
>> +++++++++++++++++++++++++++++++++++++++++ drivers/clk/rockchip/clk.c     |
>>   9 +++
>>   drivers/clk/rockchip/clk.h     |  27 ++++++++
>>   4 files changed, 184 insertions(+)
>>   create mode 100644 drivers/clk/rockchip/clk-ddr.c
>>
>> 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..5b6630d
>> --- /dev/null
>> +++ b/drivers/clk/rockchip/clk-ddr.c
>> @@ -0,0 +1,147 @@
>> +/*
>> + * 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/of.h>
>> +#include <linux/slab.h>
>> +#include <linux/io.h>
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.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)
>> +#define val_mask(width)	((1 << (width)) - 1)
> maybe use GENMASK?
     Oh, yes, we can use it. Will fix next version.
>
>> +
>> +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;
>> +
>> +	spin_lock_irqsave(ddrclk->lock, flags);
>> +
>> +	/* TODO: set ddr rate in bl31 */
> I expect this interface to be in existence and merged into the main ATF first.
>
> Right now the whole clock-type does nothing more than a simple COMPOSITE with
> added CLK_DIVIDER_READ_ONLY | CLK_MUX_READ_ONLY.
>
> Also Mike is propably still working in the so called coordinated rate change
> for clocks needing special handling on rate changes, which might provide a
> second approach to this.
     You mean there is a patch set can handle it now? Can you tell me 
the ID,
     I want to check it.
>
> So please, first of all get the ATF-interface merged and meanwhile if you need
> to read the clock-rate, just use a regular composite, with the read-only flags.
>
     My colleague are wroking on ATF code now, I agree with you, We can 
not merge
     this patch set before ATF-interface merged. And we do not need to 
add a new code
     if we only want to read ddr clock rate(we can get the ddr rate to 
read dpll), so i prefer
     to keep this function for now, and do review first, once the ATF 
code ready, we can merge
     soon(i hope :-P ) .
>> +	spin_unlock_irqrestore(ddrclk->lock, flags);
>> +
>> +	return 0;
>> +}
>> +
>> +static unsigned long
>> +rockchip_ddrclk_recalc_rate(struct clk_hw *hw,
>> +			    unsigned long parent_rate)
>> +{
>> +	struct rockchip_ddrclk *ddrclk = to_rockchip_ddrclk_hw(hw);
>> +	int val;
>> +
>> +	val = clk_readl(ddrclk->reg_base +
>> +			ddrclk->mux_offset) >> ddrclk->div_shift;
>> +	val &= val_mask(ddrclk->div_width);
>> +
>> +	return DIV_ROUND_UP_ULL((u64)parent_rate, val + 1);
> return divider_recalc_rate(...)
     got it , thank you.
>
>
> Heiko
>
>
>

-- 
Lin Huang

Re: [RFC PATCH v1 1/6] rockchip: rockchip: add new clock-type for the ddrclk

[hl]

Hi Heiko,

On 2016年06月03日 20:51, Heiko Stübner wrote:
> Am Freitag, 3. Juni 2016, 17:55:14 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>
>> ---
>> Changes in v1:
>> - None
>>
>>   drivers/clk/rockchip/Makefile  |   1 +
>>   drivers/clk/rockchip/clk-ddr.c | 147
>> +++++++++++++++++++++++++++++++++++++++++ drivers/clk/rockchip/clk.c     |
>>   9 +++
>>   drivers/clk/rockchip/clk.h     |  27 ++++++++
>>   4 files changed, 184 insertions(+)
>>   create mode 100644 drivers/clk/rockchip/clk-ddr.c
>>
>> 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..5b6630d
>> --- /dev/null
>> +++ b/drivers/clk/rockchip/clk-ddr.c
>> @@ -0,0 +1,147 @@
>> +/*
>> + * 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/of.h>
>> +#include <linux/slab.h>
>> +#include <linux/io.h>
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.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)
>> +#define val_mask(width)	((1 << (width)) - 1)
> maybe use GENMASK?
     Oh, yes, we can use it. Will fix next version.
>
>> +
>> +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;
>> +
>> +	spin_lock_irqsave(ddrclk->lock, flags);
>> +
>> +	/* TODO: set ddr rate in bl31 */
> I expect this interface to be in existence and merged into the main ATF first.
>
> Right now the whole clock-type does nothing more than a simple COMPOSITE with
> added CLK_DIVIDER_READ_ONLY | CLK_MUX_READ_ONLY.
>
> Also Mike is propably still working in the so called coordinated rate change
> for clocks needing special handling on rate changes, which might provide a
> second approach to this.
     You mean there is a patch set can handle it now? Can you tell me 
the ID,
     I want to check it.
>
> So please, first of all get the ATF-interface merged and meanwhile if you need
> to read the clock-rate, just use a regular composite, with the read-only flags.
>
     My colleague are wroking on ATF code now, I agree with you, We can 
not merge
     this patch set before ATF-interface merged. And we do not need to 
add a new code
     if we only want to read ddr clock rate(we can get the ddr rate to 
read dpll), so i prefer
     to keep this function for now, and do review first, once the ATF 
code ready, we can merge
     soon it(i hope :-P ) .
>> +	spin_unlock_irqrestore(ddrclk->lock, flags);
>> +
>> +	return 0;
>> +}
>> +
>> +static unsigned long
>> +rockchip_ddrclk_recalc_rate(struct clk_hw *hw,
>> +			    unsigned long parent_rate)
>> +{
>> +	struct rockchip_ddrclk *ddrclk = to_rockchip_ddrclk_hw(hw);
>> +	int val;
>> +
>> +	val = clk_readl(ddrclk->reg_base +
>> +			ddrclk->mux_offset) >> ddrclk->div_shift;
>> +	val &= val_mask(ddrclk->div_width);
>> +
>> +	return DIV_ROUND_UP_ULL((u64)parent_rate, val + 1);
> return divider_recalc_rate(...)
     got it , thank you.
>
>
> Heiko
>
>
>

-- 
Lin Huang

Re: [RFC PATCH v1 3/6] clk: rockchip: rk3399: add ddrc clock support

[hl]

Hi Heiko,

On 2016年06月03日 20:56, Heiko Stübner wrote:
> Am Freitag, 3. Juni 2016, 17:55:16 schrieb 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 v1:
>> - remove ddrc source CLK_IGNORE_UNUSED flag, Suggestion by Doug
>> - move clk_ddrc and clk_ddrc_dpll_src to critical, Suggestion by Doug
>>
>>   drivers/clk/rockchip/clk-rk3399.c | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/clk/rockchip/clk-rk3399.c
>> b/drivers/clk/rockchip/clk-rk3399.c index f1d8e44..29afb88 100644
>> --- a/drivers/clk/rockchip/clk-rk3399.c
>> +++ b/drivers/clk/rockchip/clk-rk3399.c
> [...]
>
>> @@ -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),
>>   };
> as said in the other patch, just make this a regular COMPOSITE_NOGATE with
> CLK_DIVIDER_READ_ONLY | CLK_MUX_READ_ONLY until that interface to the ATF
> exists and is approved.
>
> That way you can still read back the clock rate without anything changing the
> clock-rate, but we don't need to add duplicate code for it.
>
>
>>   static struct rockchip_clk_branch rk3399_clk_pmu_branches[] __initdata = {
>> @@ -1487,6 +1503,10 @@ static const char *const rk3399_cru_critical_clocks[]
>> __initconst = { "gpll_hclk_perilp1_src",
>>   	"gpll_aclk_perilp0_src",
>>   	"gpll_aclk_perihp_src",
>> +
>> +	/* ddrc */
>> +	"clk_ddrc_dpll_src",
> Why does your clk_ddrc_dpll_src need a separate critical entry. Any code
> changing the clk_ddrc parent should make sure the new parent is enabled. (The
> clock-framework of course does this already).
     Okay, thank you.
>
>> +	"clk_ddrc",
>>   };
>>
>>   static const char *const rk3399_pmucru_critical_clocks[] __initconst = {
>
> Heiko
>
>
>
>

-- 
Lin Huang

Re: [RFC PATCH v1 4/6] PM / devfreq: event: support rockchip dfi controller

[hl]

On 2016年06月03日 18:26, Chanwoo Choi wrote:
> Hi Lin,
>
> I add the some comment on below. If you modify it,
> You can add my acked-by tag. Looks good to me.
Thanks for you reviewing, i will update the code folloiwing your comment.
> Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
>
> Also, I'd like you to add me to mail thread
> on next version because I'm supporter of devfreq-event.
I am sorry for missing you mail in before patch:-[ , will add you to 
mail thread next vesion.
> On 2016년 06월 03일 18:55, 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>
>> ---
>> Changes in v1:
>> - NOne
>>
>>   drivers/devfreq/event/Kconfig        |   7 +
>>   drivers/devfreq/event/Makefile       |   1 +
>>   drivers/devfreq/event/rockchip-dfi.c | 265 +++++++++++++++++++++++++++++++++++
>>   3 files changed, 273 insertions(+)
>>   create mode 100644 drivers/devfreq/event/rockchip-dfi.c
>>
>> diff --git a/drivers/devfreq/event/Kconfig b/drivers/devfreq/event/Kconfig
>> index 1e8b4f4..6ebdc13 100644
>> --- a/drivers/devfreq/event/Kconfig
>> +++ b/drivers/devfreq/event/Kconfig
>> @@ -30,4 +30,11 @@ config DEVFREQ_EVENT_EXYNOS_PPMU
>>   	  (Platform Performance Monitoring Unit) counters to estimate the
>>   	  utilization of each module.
>>   
>> +config DEVFREQ_EVENT_ROCKCHIP_DFI
>> +	bool "ROCKCHIP DFI DEVFREQ event Driver"
>> +	depends on ARCH_ROCKCHIP
>> +	help
>> +	  This add the devfreq-event driver for Rockchip SoC. It provides DFI
> You better to full name of 'DFI' abbreviation as following:
> - DFI (Dxxx Fxxx Ixxx)
>
>> +	  driver to count ddr load.
>> +
>>   endif # PM_DEVFREQ_EVENT
>> diff --git a/drivers/devfreq/event/Makefile b/drivers/devfreq/event/Makefile
>> index 3d6afd3..dda7090 100644
>> --- a/drivers/devfreq/event/Makefile
>> +++ b/drivers/devfreq/event/Makefile
>> @@ -2,3 +2,4 @@
>>   
>>   obj-$(CONFIG_DEVFREQ_EVENT_EXYNOS_NOCP) += exynos-nocp.o
>>   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..e3b020f9
>> --- /dev/null
>> +++ b/drivers/devfreq/event/rockchip-dfi.c
>> @@ -0,0 +1,265 @@
>> +/*
>> + * 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(info->clk);
> I prefer to change the function as following. I add the comment on below in probe()
> - clk_disable -> clk_disable_unprepare()
>
>
>> +
>> +	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_enable(info->clk);
> I prefer to change the function as following. I add the comment on below in probe()
> - clk_enable -> clk_prepare_enable()
>
>> +	if (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;
>> +	int ret;
>> +	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)) {
>> +		ret = PTR_ERR(data->edev);
>> +		dev_err(&pdev->dev,
>> +			"failed to add devfreq-event device\n");
>> +		return ret;
> You can simply return the PTR_ERR(data->edev) without 'ret' variable as following:
>
> 	return PTR_ERR(data->edev);
>
>
>> +	}
>> +
>> +	ret = clk_prepare_enable(data->clk);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to enable clk: %d\n", ret);
>> +		clk_disable_unprepare(data->clk);
>> +		return ret;
>> +	}
> The following two functions handle the clock. So, rockchip_dfi_probe()
> don't need to enable the clock. Just pass the role of clock control to the following functions.
> Because of calling the twice of enable function of clock, the usage count of clock
> is mismatch when disabling the clock.
> - rockchip_dfi_enable(struct devfreq_event_dev *edev) enable the clock.
> - rockchip_dfi_disable(struct devfreq_event_dev *edev) disable the clock.
>
>
>> +
>> +	platform_set_drvdata(pdev, data);
>> +
>> +	return 0;
>> +}
>> +
>> +static int rockchip_dfi_remove(struct platform_device *pdev)
>> +{
>> +	return 0;
>> +}
> If the rockchip_dfi_remove() don't do anything, you can remove it
>
>> +
>> +static struct platform_driver rockchip_dfi_driver = {
>> +	.probe	= rockchip_dfi_probe,
>> +	.remove	= rockchip_dfi_remove,
> ditto.
>
>> +	.driver = {
>> +		.name	= "rockchip-dfi",
>> +		.of_match_table = rockchip_dfi_id_match,
>> +	},
>> +};
>> +module_platform_driver(rockchip_dfi_driver);
>> +
>> +MODULE_LICENSE("GPL v2");
> You need to add MODULE_AUTHOR("").
>
>> +MODULE_DESCRIPTION("Rockchip dfi driver");
>>
> Remove unneeded blank line
>
> Regards,
> Chanwoo Choi
>
>
>
>

-- 
Lin Huang

Re: [RFC PATCH v1 4/6] PM / devfreq: event: support rockchip dfi controller

[hl]

Hi Thierry,

On 2016年06月04日 00:54, Thierry Reding wrote:
> On Fri, Jun 03, 2016 at 05:55:17PM +0800, Lin Huang wrote:
> [...]
>> +	ret = clk_prepare_enable(data->clk);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to enable clk: %d\n", ret);
>> +		clk_disable_unprepare(data->clk);
>> +		return ret;
>> +	}
> This is going to give you a large WARN. clk_prepare_enable() already
> leaves the clock in a proper state when it fails (i.e. it calls
> clk_unprepare() if the clk_enable() part failed), so calling
> clk_disable_unprepare() upon failure is going to unbalance the
> reference counts.
Thanks for reminding, i will fix it next version.
>
> Thierry

-- 
Lin Huang