[RFC PATCH 00/10] Add support for drm/rockchip to dynamically control the DDR frequency.

[RFC PATCH 00/10] Add support for drm/rockchip to dynamically control the DDR frequency.

[Enric Balletbo i Serra]

Dear all,

The rk3399 platform has a DFI controller that can monitor DDR load and a
DMC driver that talks with the TF-A (Trusted Firmware-A) to dynamically
set the DDR frequency with following flow.

             kernel                          Trusted Firmware-A
                                                  (bl31)
        monitor ddr load
                |
                |
        get_target_rate
                |
                |           pass rate to TF-A
        clk_set_rate(ddr) --------------------->run ddr dvs flow
                                                    |
                                                    |
                 <------------------------------end ddr dvs flow
                |
                |
              return

These patches add support for devfreq to dynamically control the DDR
frequency into the drm rockchip driver. By default it uses the
'simple_ondemand' governor which can adjust the frequency based on the
DDR load.

Although the patches were present in the chromeos kernel for long time I
am sending this as a RFC because I have doubts on some of them,
specially patch 4/10 that tries to sync the TF-A with the kernel using a
mutex, to be honest I did not find another way to do it so feedback is
more than welcome.

The patches apply on top of current 4.17-rc5 and depends on the
following series to apply cleanly.

 1. https://lkml.org/lkml/2018/5/9/464
 2. https://lkml.org/lkml/2018/4/23/354

Waiting for your feedback.

Best regards,
 Enric


Derek Basehore (3):
  devfreq: rk3399_dmc / clk: rockchip: Sync with vblank in the kernel
    for DDRfreq.
  devfreq: rk3399_dmc / clk: rockchip: Disable DDR clk timeout on
    suspend.
  clk: rockchip: set clk-ddr to GET_RATE_NOCACHE.

Enric Balletbo i Serra (3):
  devfreq: rockchip-dfi: Move GRF definitions to a common place.
  dt-bindings: devfreq: rk3399_dmc: Add rockchip,pmu phandle.
  devfreq: rk3399_dmc: Pass ODT and auto power down parameters to TF-A.

Lin Huang (3):
  devfreq: rk3399_dmc / rockchip: pm_domains: Register notify to DMC
    driver.
  arm64: dts: rk3399: Add dfi and dmc nodes.
  arm64: dts: rockchip: Enable dmc and dfi nodes on gru.

Sean Paul (1):
  drm: rockchip: Add DDR devfreq support.

 .../bindings/devfreq/rk3399_dmc.txt           |   2 +
 .../rockchip/rk3399-dram-default-timing.dtsi  |  38 ++++
 arch/arm64/boot/dts/rockchip/rk3399-dram.h    |  73 +++++++
 arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi  |  21 ++
 .../boot/dts/rockchip/rk3399-op1-opp.dtsi     |  29 +++
 arch/arm64/boot/dts/rockchip/rk3399.dtsi      |  22 +-
 drivers/clk/rockchip/clk-ddr.c                | 159 ++++++++++++---
 drivers/clk/rockchip/clk.c                    |   2 +-
 drivers/clk/rockchip/clk.h                    |   3 +-
 drivers/devfreq/event/rockchip-dfi.c          |  23 +--
 drivers/devfreq/rk3399_dmc.c                  | 191 +++++++++++++++---
 drivers/devfreq/rk3399_dmc_priv.h             |  16 ++
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c   |  46 +++++
 drivers/gpu/drm/rockchip/rockchip_drm_drv.h   |   9 +
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c    |  35 ++++
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c   |  34 ++++
 drivers/soc/rockchip/pm_domains.c             |  36 ++++
 include/soc/rockchip/rk3399_dmc.h             |  44 ++++
 include/soc/rockchip/rk3399_grf.h             |  21 ++
 include/soc/rockchip/rockchip_sip.h           |   1 +
 20 files changed, 734 insertions(+), 71 deletions(-)
 create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-dram-default-timing.dtsi
 create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-dram.h
 create mode 100644 drivers/devfreq/rk3399_dmc_priv.h
 create mode 100644 include/soc/rockchip/rk3399_dmc.h
 create mode 100644 include/soc/rockchip/rk3399_grf.h

-- 
2.17.0

[RFC PATCH 01/10] devfreq: rockchip-dfi: Move GRF definitions to a common place.

[Enric Balletbo i Serra]

Some rk3399 GRF (Generic Register Files) definitions can be used for
different drivers. Move these definitions to a common include so we
don't need to duplicate these definitions.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 drivers/devfreq/event/rockchip-dfi.c | 23 +++++++----------------
 include/soc/rockchip/rk3399_grf.h    | 21 +++++++++++++++++++++
 2 files changed, 28 insertions(+), 16 deletions(-)
 create mode 100644 include/soc/rockchip/rk3399_grf.h

diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
index 22b113363ffc..2fbbcbeb644f 100644
--- a/drivers/devfreq/event/rockchip-dfi.c
+++ b/drivers/devfreq/event/rockchip-dfi.c
@@ -26,6 +26,8 @@
 #include <linux/list.h>
 #include <linux/of.h>
 
+#include <soc/rockchip/rk3399_grf.h>
+
 #define RK3399_DMC_NUM_CH	2
 
 /* DDRMON_CTRL */
@@ -43,18 +45,6 @@
 #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;
@@ -83,16 +73,17 @@ static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
 	u32 ddr_type;
 
 	/* get ddr type */
-	regmap_read(info->regmap_pmu, PMUGRF_OS_REG2, &val);
-	ddr_type = (val >> DDRTYPE_SHIFT) & DDRTYPE_MASK;
+	regmap_read(info->regmap_pmu, RK3399_PMUGRF_OS_REG2, &val);
+	ddr_type = (val >> RK3399_PMUGRF_DDRTYPE_SHIFT) &
+		    RK3399_PMUGRF_DDRTYPE_MASK;
 
 	/* clear DDRMON_CTRL setting */
 	writel_relaxed(CLR_DDRMON_CTRL, dfi_regs + DDRMON_CTRL);
 
 	/* set ddr type to dfi */
-	if (ddr_type == LPDDR3)
+	if (ddr_type == RK3399_PMUGRF_DDRTYPE_LPDDR3)
 		writel_relaxed(LPDDR3_EN, dfi_regs + DDRMON_CTRL);
-	else if (ddr_type == LPDDR4)
+	else if (ddr_type == RK3399_PMUGRF_DDRTYPE_LPDDR4)
 		writel_relaxed(LPDDR4_EN, dfi_regs + DDRMON_CTRL);
 
 	/* enable count, use software mode */
diff --git a/include/soc/rockchip/rk3399_grf.h b/include/soc/rockchip/rk3399_grf.h
new file mode 100644
index 000000000000..0f94034e2e9a
--- /dev/null
+++ b/include/soc/rockchip/rk3399_grf.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Rockchip Generic Register Files definitions
+ *
+ * Copyright (c) 2018, Collabora Ltd.
+ * Author: Enric Balletbo i Serra <enric.balletbo@collabora.com>
+ */
+
+#ifndef __SOC_RK3399_GRF_H
+#define __SOC_RK3399_GRF_H
+
+/* PMU GRF Registers */
+#define RK3399_PMUGRF_OS_REG2		0x308
+#define RK3399_PMUGRF_DDRTYPE_SHIFT	13
+#define RK3399_PMUGRF_DDRTYPE_MASK	7
+#define RK3399_PMUGRF_DDRTYPE_DDR3	3
+#define RK3399_PMUGRF_DDRTYPE_LPDDR2	5
+#define RK3399_PMUGRF_DDRTYPE_LPDDR3	6
+#define RK3399_PMUGRF_DDRTYPE_LPDDR4	7
+
+#endif
-- 
2.17.0

[RFC PATCH 02/10] dt-bindings: devfreq: rk3399_dmc: Add rockchip,pmu phandle.

[Enric Balletbo i Serra]

The Rockchip DMC (Dynamic Memory Interface) needs to access to the PMU
general register files to know the DRAM type, so add a phandle to the
syscon that manages these registers.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt b/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
index 0ec68141f85a..951789c0cdd6 100644
--- a/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
+++ b/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
@@ -12,6 +12,8 @@ Required properties:
 			 for details.
 - center-supply:	 DMC supply node.
 - status:		 Marks the node enabled/disabled.
+- rockchip,pmu:		 Phandle to the syscon managing the "PMU general register
+			 files".
 
 Optional properties:
 - interrupts:		 The CPU interrupt number. The interrupt specifier
-- 
2.17.0

[RFC PATCH 03/10] devfreq: rk3399_dmc: Pass ODT and auto power down parameters to TF-A.

[Enric Balletbo i Serra]

Trusted Firmware-A (TF-A) for rk3399 implements a SiP call to get the
on-die termination (ODT) and auto power down parameters from kernel,
this patch adds the functionality to do this. Also, if DDR clock
frequency is lower than the on-die termination (ODT) disable frequency
this driver should disable the DDR ODT.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 drivers/devfreq/rk3399_dmc.c        | 50 ++++++++++++++++++++++++++++-
 include/soc/rockchip/rockchip_sip.h |  1 +
 2 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
index d5c03e5abe13..cc1bbca3fb15 100644
--- a/drivers/devfreq/rk3399_dmc.c
+++ b/drivers/devfreq/rk3399_dmc.c
@@ -18,14 +18,17 @@
 #include <linux/devfreq.h>
 #include <linux/devfreq-event.h>
 #include <linux/interrupt.h>
+#include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/pm_opp.h>
+#include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/rwsem.h>
 #include <linux/suspend.h>
 
+#include <soc/rockchip/rk3399_grf.h>
 #include <soc/rockchip/rockchip_sip.h>
 
 struct dram_timing {
@@ -69,8 +72,11 @@ struct rk3399_dmcfreq {
 	struct mutex lock;
 	struct dram_timing timing;
 	struct regulator *vdd_center;
+	struct regmap *regmap_pmu;
 	unsigned long rate, target_rate;
 	unsigned long volt, target_volt;
+	unsigned int odt_dis_freq;
+	int odt_pd_arg0, odt_pd_arg1;
 };
 
 static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
@@ -80,6 +86,8 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
 	struct dev_pm_opp *opp;
 	unsigned long old_clk_rate = dmcfreq->rate;
 	unsigned long target_volt, target_rate;
+	struct arm_smccc_res res;
+	int dram_flag;
 	int err;
 
 	opp = devfreq_recommended_opp(dev, freq, flags);
@@ -95,6 +103,15 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
 
 	mutex_lock(&dmcfreq->lock);
 
+	dram_flag = 0;
+	if (target_rate >= dmcfreq->odt_dis_freq)
+		dram_flag = 1;
+
+	arm_smccc_smc(ROCKCHIP_SIP_DRAM_FREQ, dmcfreq->odt_pd_arg0,
+		      dmcfreq->odt_pd_arg1,
+		      ROCKCHIP_SIP_CONFIG_DRAM_SET_ODT_PD,
+		      dram_flag, 0, 0, 0, &res);
+
 	/*
 	 * If frequency scaling from low to high, adjust voltage first.
 	 * If frequency scaling from high to low, adjust frequency first.
@@ -294,11 +311,13 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
 {
 	struct arm_smccc_res res;
 	struct device *dev = &pdev->dev;
-	struct device_node *np = pdev->dev.of_node;
+	struct device_node *np = pdev->dev.of_node, *node;
 	struct rk3399_dmcfreq *data;
 	int ret, index, size;
 	uint32_t *timing;
 	struct dev_pm_opp *opp;
+	u32 ddr_type;
+	u32 val;
 
 	data = devm_kzalloc(dev, sizeof(struct rk3399_dmcfreq), GFP_KERNEL);
 	if (!data)
@@ -334,6 +353,29 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	/* 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);
+	}
+
+	/* Get DDR type */
+	regmap_read(data->regmap_pmu, RK3399_PMUGRF_OS_REG2, &val);
+	ddr_type = (val >> RK3399_PMUGRF_DDRTYPE_SHIFT) &
+		    RK3399_PMUGRF_DDRTYPE_MASK;
+
+	/* Get the odt_dis_freq parameter in function of the DDR type */
+	if (ddr_type == RK3399_PMUGRF_DDRTYPE_DDR3)
+		data->odt_dis_freq = data->timing.ddr3_odt_dis_freq;
+	else if (ddr_type == RK3399_PMUGRF_DDRTYPE_LPDDR3)
+		data->odt_dis_freq = data->timing.lpddr3_odt_dis_freq;
+	else if (ddr_type == RK3399_PMUGRF_DDRTYPE_LPDDR4)
+		data->odt_dis_freq = data->timing.lpddr4_odt_dis_freq;
+	else
+		return -EINVAL;
+
 	/*
 	 * Get dram timing and pass it to arm trust firmware,
 	 * the dram drvier in arm trust firmware will get these
@@ -358,6 +400,12 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
 		      ROCKCHIP_SIP_CONFIG_DRAM_INIT,
 		      0, 0, 0, 0, &res);
 
+	data->odt_pd_arg0 = (data->timing.sr_idle & 0xff) |
+			    ((data->timing.sr_mc_gate_idle & 0xff) << 8) |
+			    ((data->timing.standby_idle & 0xffff) << 16);
+	data->odt_pd_arg1 = (data->timing.pd_idle & 0xfff) |
+			    ((data->timing.srpd_lite_idle & 0xfff) << 16);
+
 	/*
 	 * We add a devfreq driver to our parent since it has a device tree node
 	 * with operating points.
diff --git a/include/soc/rockchip/rockchip_sip.h b/include/soc/rockchip/rockchip_sip.h
index 7e28092c4d3d..ad9482c56797 100644
--- a/include/soc/rockchip/rockchip_sip.h
+++ b/include/soc/rockchip/rockchip_sip.h
@@ -23,5 +23,6 @@
 #define ROCKCHIP_SIP_CONFIG_DRAM_GET_RATE	0x05
 #define ROCKCHIP_SIP_CONFIG_DRAM_CLR_IRQ	0x06
 #define ROCKCHIP_SIP_CONFIG_DRAM_SET_PARAM	0x07
+#define ROCKCHIP_SIP_CONFIG_DRAM_SET_ODT_PD	0x08
 
 #endif
-- 
2.17.0

[RFC PATCH 04/10] devfreq: rk3399_dmc / rockchip: pm_domains: Register notify to DMC driver.

[Enric Balletbo i Serra]

From: Lin Huang <hl@rock-chips.com>

The DMC (Dynamic Memory Interface) controller does a SiP call to the
Trusted Firmware-A (TF-A) to change the DDR clock frequency. When this
happens the TF-A writes to the PMU bus idle request register
(PMU_BUS_IDLE_REQ) but at the same time it is possible that the Rockchip
power domain driver writes to the same register. So, add a notification
mechanism to ensure that the DMC and the PD driver does not access to this
register at the same time.

Signed-off-by: Lin Huang <hl@rock-chips.com>
[rewrite commit message]
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
As I explained in the cover letter I have doubts regarding this patch
but I did not find another way to do it. So I will appreciate any
feedback on this.

 drivers/devfreq/rk3399_dmc.c      |  7 ++++++
 drivers/soc/rockchip/pm_domains.c | 36 +++++++++++++++++++++++++++++++
 include/soc/rockchip/rk3399_dmc.h | 14 ++++++++++++
 3 files changed, 57 insertions(+)
 create mode 100644 include/soc/rockchip/rk3399_dmc.h

diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
index cc1bbca3fb15..2c4985a501cb 100644
--- a/drivers/devfreq/rk3399_dmc.c
+++ b/drivers/devfreq/rk3399_dmc.c
@@ -28,6 +28,7 @@
 #include <linux/rwsem.h>
 #include <linux/suspend.h>
 
+#include <soc/rockchip/rk3399_dmc.h>
 #include <soc/rockchip/rk3399_grf.h>
 #include <soc/rockchip/rockchip_sip.h>
 
@@ -443,6 +444,12 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
 	data->dev = dev;
 	platform_set_drvdata(pdev, data);
 
+	rockchip_pm_register_dmcfreq_notifier(data->devfreq);
+	if (ret < 0) {
+		dev_err(dev, "Failed to register dmcfreq notifier\n");
+		return ret;
+	}
+
 	return 0;
 }
 
diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c
index 53efc386b1ad..b0e66f24b3e3 100644
--- a/drivers/soc/rockchip/pm_domains.c
+++ b/drivers/soc/rockchip/pm_domains.c
@@ -8,6 +8,7 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/devfreq.h>
 #include <linux/io.h>
 #include <linux/iopoll.h>
 #include <linux/err.h>
@@ -76,9 +77,13 @@ struct rockchip_pmu {
 	const struct rockchip_pmu_info *info;
 	struct mutex mutex; /* mutex lock for pmu */
 	struct genpd_onecell_data genpd_data;
+	struct devfreq *devfreq;
+	struct notifier_block dmc_nb;
 	struct generic_pm_domain *domains[];
 };
 
+static struct rockchip_pmu *dmc_pmu;
+
 #define to_rockchip_pd(gpd) container_of(gpd, struct rockchip_pm_domain, genpd)
 
 #define DOMAIN(pwr, status, req, idle, ack, wakeup)	\
@@ -601,6 +606,35 @@ static int rockchip_pm_add_subdomain(struct rockchip_pmu *pmu,
 	return error;
 }
 
+static int rk3399_dmcfreq_notify(struct notifier_block *nb,
+				 unsigned long event, void *data)
+{
+	if (event == DEVFREQ_PRECHANGE)
+		mutex_lock(&dmc_pmu->mutex);
+	else if (event == DEVFREQ_POSTCHANGE)
+		mutex_unlock(&dmc_pmu->mutex);
+
+	return NOTIFY_OK;
+}
+
+int rockchip_pm_register_dmcfreq_notifier(struct devfreq *devfreq)
+{
+	int ret;
+
+	if (!dmc_pmu)
+		return -EPROBE_DEFER;
+
+	dmc_pmu->devfreq = devfreq;
+	dmc_pmu->dmc_nb.notifier_call = rk3399_dmcfreq_notify;
+	ret = devm_devfreq_register_notifier(devfreq->dev.parent,
+					     dmc_pmu->devfreq,
+					     &dmc_pmu->dmc_nb,
+					     DEVFREQ_TRANSITION_NOTIFIER);
+
+	return ret;
+}
+EXPORT_SYMBOL(rockchip_pm_register_dmcfreq_notifier);
+
 static int rockchip_pm_domain_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -694,6 +728,8 @@ static int rockchip_pm_domain_probe(struct platform_device *pdev)
 		goto err_out;
 	}
 
+	dmc_pmu = pmu;
+
 	return 0;
 
 err_out:
diff --git a/include/soc/rockchip/rk3399_dmc.h b/include/soc/rockchip/rk3399_dmc.h
new file mode 100644
index 000000000000..031a62607f61
--- /dev/null
+++ b/include/soc/rockchip/rk3399_dmc.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2016-2018, Fuzhou Rockchip Electronics Co., Ltd
+ * Author: Lin Huang <hl@rock-chips.com>
+ */
+
+#ifndef __SOC_RK3399_DMC_H
+#define __SOC_RK3399_DMC_H
+
+#include <linux/devfreq.h>
+
+int rockchip_pm_register_dmcfreq_notifier(struct devfreq *devfreq);
+
+#endif
-- 
2.17.0

[RFC PATCH 05/10] devfreq: rk3399_dmc / clk: rockchip: Sync with vblank in the kernel for DDRfreq.

[Enric Balletbo i Serra]

From: Derek Basehore <dbasehore@chromium.org>

This changes the kernel to sync with vblank for the display in the
kernel. This was done in Trusted Firmware-A (TF-A) before, but that locks
up one CPU for up to one display frame (1/60 second). That's bad for
performance and power, so this moves waiting to the kernel where the
waiting thread can properly wait on a completion.

Signed-off-by: Derek Basehore <dbasehore@chromium.org>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 drivers/clk/rockchip/clk-ddr.c    | 142 +++++++++++++++++++++++++-----
 drivers/clk/rockchip/clk.c        |   2 +-
 drivers/clk/rockchip/clk.h        |   3 +-
 drivers/devfreq/rk3399_dmc.c      | 124 ++++++++++++++++++++------
 drivers/devfreq/rk3399_dmc_priv.h |  15 ++++
 include/soc/rockchip/rk3399_dmc.h |  30 +++++++
 6 files changed, 264 insertions(+), 52 deletions(-)
 create mode 100644 drivers/devfreq/rk3399_dmc_priv.h

diff --git a/drivers/clk/rockchip/clk-ddr.c b/drivers/clk/rockchip/clk-ddr.c
index e8075359366b..707be1bd8910 100644
--- a/drivers/clk/rockchip/clk-ddr.c
+++ b/drivers/clk/rockchip/clk-ddr.c
@@ -17,7 +17,9 @@
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/io.h>
+#include <linux/ktime.h>
 #include <linux/slab.h>
+#include <soc/rockchip/rk3399_dmc.h>
 #include <soc/rockchip/rockchip_sip.h>
 #include "clk.h"
 
@@ -30,39 +32,104 @@ struct rockchip_ddrclk {
 	int		div_shift;
 	int		div_width;
 	int		ddr_flag;
-	spinlock_t	*lock;
+	unsigned long	cached_rate;
+	struct work_struct set_rate_work;
+	struct mutex	lock;
+	struct raw_notifier_head sync_chain;
 };
 
 #define to_rockchip_ddrclk_hw(hw) container_of(hw, struct rockchip_ddrclk, hw)
+#define DMC_DEFAULT_TIMEOUT_NS		NSEC_PER_SEC
+#define DDRCLK_SET_RATE_MAX_RETRIES	3
 
-static int rockchip_ddrclk_sip_set_rate(struct clk_hw *hw, unsigned long drate,
-					unsigned long prate)
+static void rockchip_ddrclk_set_rate_func(struct work_struct *work)
 {
-	struct rockchip_ddrclk *ddrclk = to_rockchip_ddrclk_hw(hw);
-	unsigned long flags;
+	struct rockchip_ddrclk *ddrclk = container_of(work,
+			struct rockchip_ddrclk, set_rate_work);
+	ktime_t timeout = ktime_add_ns(ktime_get(), DMC_DEFAULT_TIMEOUT_NS);
 	struct arm_smccc_res res;
+	int ret, i;
+
+	mutex_lock(&ddrclk->lock);
+	for (i = 0; i < DDRCLK_SET_RATE_MAX_RETRIES; i++) {
+		ret = raw_notifier_call_chain(&ddrclk->sync_chain, 0, &timeout);
+		if (ret == NOTIFY_BAD)
+			goto out;
+
+		/*
+		 * Check the timeout with irqs disabled. This is so we don't get
+		 * preempted after checking the timeout. That could cause things
+		 * like garbage values for the display if we change the DDR rate
+		 * at the wrong time.
+		 */
+		local_irq_disable();
+		if (ktime_after(ktime_add_ns(ktime_get(), DMC_MIN_VBLANK_NS),
+				timeout)) {
+			local_irq_enable();
+			continue;
+		}
+
+		arm_smccc_smc(ROCKCHIP_SIP_DRAM_FREQ, ddrclk->cached_rate, 0,
+			      ROCKCHIP_SIP_CONFIG_DRAM_SET_RATE,
+			      0, 0, 0, 0, &res);
+		local_irq_enable();
+		break;
+	}
+out:
+	mutex_unlock(&ddrclk->lock);
+}
 
-	spin_lock_irqsave(ddrclk->lock, flags);
-	arm_smccc_smc(ROCKCHIP_SIP_DRAM_FREQ, drate, 0,
-		      ROCKCHIP_SIP_CONFIG_DRAM_SET_RATE,
-		      0, 0, 0, 0, &res);
-	spin_unlock_irqrestore(ddrclk->lock, flags);
+int rockchip_ddrclk_register_sync_nb(struct clk *clk, struct notifier_block *nb)
+{
+	struct clk_hw *hw = __clk_get_hw(clk);
+	struct rockchip_ddrclk *ddrclk;
+	int ret;
 
-	return res.a0;
+	if (!hw || !nb)
+		return -EINVAL;
+
+	ddrclk = to_rockchip_ddrclk_hw(hw);
+	if (!ddrclk)
+		return -EINVAL;
+
+	mutex_lock(&ddrclk->lock);
+	ret = raw_notifier_chain_register(&ddrclk->sync_chain, nb);
+	mutex_unlock(&ddrclk->lock);
+
+	return ret;
 }
+EXPORT_SYMBOL_GPL(rockchip_ddrclk_register_sync_nb);
 
-static unsigned long
-rockchip_ddrclk_sip_recalc_rate(struct clk_hw *hw,
-				unsigned long parent_rate)
+int rockchip_ddrclk_unregister_sync_nb(struct clk *clk,
+				       struct notifier_block *nb)
 {
-	struct arm_smccc_res res;
+	struct clk_hw *hw = __clk_get_hw(clk);
+	struct rockchip_ddrclk *ddrclk;
+	int ret;
 
-	arm_smccc_smc(ROCKCHIP_SIP_DRAM_FREQ, 0, 0,
-		      ROCKCHIP_SIP_CONFIG_DRAM_GET_RATE,
-		      0, 0, 0, 0, &res);
+	if (!hw || !nb)
+		return -EINVAL;
 
-	return res.a0;
+	ddrclk = to_rockchip_ddrclk_hw(hw);
+	if (!ddrclk)
+		return -EINVAL;
+
+	mutex_lock(&ddrclk->lock);
+	ret = raw_notifier_chain_unregister(&ddrclk->sync_chain, nb);
+	mutex_unlock(&ddrclk->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(rockchip_ddrclk_unregister_sync_nb);
+
+void rockchip_ddrclk_wait_set_rate(struct clk *clk)
+{
+	struct clk_hw *hw = __clk_get_hw(clk);
+	struct rockchip_ddrclk *ddrclk = to_rockchip_ddrclk_hw(hw);
+
+	flush_work(&ddrclk->set_rate_work);
 }
+EXPORT_SYMBOL_GPL(rockchip_ddrclk_wait_set_rate);
 
 static long rockchip_ddrclk_sip_round_rate(struct clk_hw *hw,
 					   unsigned long rate,
@@ -77,6 +144,30 @@ static long rockchip_ddrclk_sip_round_rate(struct clk_hw *hw,
 	return res.a0;
 }
 
+static int rockchip_ddrclk_sip_set_rate(struct clk_hw *hw, unsigned long drate,
+					unsigned long prate)
+{
+	struct rockchip_ddrclk *ddrclk = to_rockchip_ddrclk_hw(hw);
+	long rate;
+
+	rate = rockchip_ddrclk_sip_round_rate(hw, drate, &prate);
+	if (rate < 0)
+		return rate;
+
+	ddrclk->cached_rate = rate;
+	queue_work(system_highpri_wq, &ddrclk->set_rate_work);
+	return 0;
+}
+
+static unsigned long
+rockchip_ddrclk_sip_recalc_rate(struct clk_hw *hw,
+				unsigned long parent_rate)
+{
+	struct rockchip_ddrclk *ddrclk = to_rockchip_ddrclk_hw(hw);
+
+	return ddrclk->cached_rate;
+}
+
 static u8 rockchip_ddrclk_get_parent(struct clk_hw *hw)
 {
 	struct rockchip_ddrclk *ddrclk = to_rockchip_ddrclk_hw(hw);
@@ -105,12 +196,12 @@ struct clk *rockchip_clk_register_ddrclk(const char *name, int flags,
 					 u8 num_parents, int mux_offset,
 					 int mux_shift, int mux_width,
 					 int div_shift, int div_width,
-					 int ddr_flag, void __iomem *reg_base,
-					 spinlock_t *lock)
+					 int ddr_flag, void __iomem *reg_base)
 {
 	struct rockchip_ddrclk *ddrclk;
 	struct clk_init_data init;
 	struct clk *clk;
+	struct arm_smccc_res res;
 
 	ddrclk = kzalloc(sizeof(*ddrclk), GFP_KERNEL);
 	if (!ddrclk)
@@ -134,7 +225,6 @@ struct clk *rockchip_clk_register_ddrclk(const char *name, int flags,
 	}
 
 	ddrclk->reg_base = reg_base;
-	ddrclk->lock = lock;
 	ddrclk->hw.init = &init;
 	ddrclk->mux_offset = mux_offset;
 	ddrclk->mux_shift = mux_shift;
@@ -142,6 +232,14 @@ struct clk *rockchip_clk_register_ddrclk(const char *name, int flags,
 	ddrclk->div_shift = div_shift;
 	ddrclk->div_width = div_width;
 	ddrclk->ddr_flag = ddr_flag;
+	mutex_init(&ddrclk->lock);
+	INIT_WORK(&ddrclk->set_rate_work, rockchip_ddrclk_set_rate_func);
+	RAW_INIT_NOTIFIER_HEAD(&ddrclk->sync_chain);
+
+	arm_smccc_smc(ROCKCHIP_SIP_DRAM_FREQ, 0, 0,
+		      ROCKCHIP_SIP_CONFIG_DRAM_GET_RATE,
+		      0, 0, 0, 0, &res);
+	ddrclk->cached_rate = res.a0;
 
 	clk = clk_register(NULL, &ddrclk->hw);
 	if (IS_ERR(clk))
diff --git a/drivers/clk/rockchip/clk.c b/drivers/clk/rockchip/clk.c
index 3cd8ad59e0b7..0e208d95d048 100644
--- a/drivers/clk/rockchip/clk.c
+++ b/drivers/clk/rockchip/clk.c
@@ -547,7 +547,7 @@ void __init rockchip_clk_register_branches(
 				list->muxdiv_offset, list->mux_shift,
 				list->mux_width, list->div_shift,
 				list->div_width, list->div_flags,
-				ctx->reg_base, &ctx->lock);
+				ctx->reg_base);
 			break;
 		}
 
diff --git a/drivers/clk/rockchip/clk.h b/drivers/clk/rockchip/clk.h
index ef601dded32c..5e4ce49ef337 100644
--- a/drivers/clk/rockchip/clk.h
+++ b/drivers/clk/rockchip/clk.h
@@ -326,8 +326,7 @@ struct clk *rockchip_clk_register_ddrclk(const char *name, int flags,
 					 u8 num_parents, int mux_offset,
 					 int mux_shift, int mux_width,
 					 int div_shift, int div_width,
-					 int ddr_flags, void __iomem *reg_base,
-					 spinlock_t *lock);
+					 int ddr_flags, void __iomem *reg_base);
 
 #define ROCKCHIP_INVERTER_HIWORD_MASK	BIT(0)
 
diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
index 2c4985a501cb..c174d13afe92 100644
--- a/drivers/devfreq/rk3399_dmc.c
+++ b/drivers/devfreq/rk3399_dmc.c
@@ -32,6 +32,8 @@
 #include <soc/rockchip/rk3399_grf.h>
 #include <soc/rockchip/rockchip_sip.h>
 
+#include "rk3399_dmc_priv.h"
+
 struct dram_timing {
 	unsigned int ddr3_speed_bin;
 	unsigned int pd_idle;
@@ -71,6 +73,7 @@ struct rk3399_dmcfreq {
 	struct clk *dmc_clk;
 	struct devfreq_event_dev *edev;
 	struct mutex lock;
+	struct mutex en_lock;
 	struct dram_timing timing;
 	struct regulator *vdd_center;
 	struct regmap *regmap_pmu;
@@ -78,6 +81,8 @@ struct rk3399_dmcfreq {
 	unsigned long volt, target_volt;
 	unsigned int odt_dis_freq;
 	int odt_pd_arg0, odt_pd_arg1;
+	int num_sync_nb;
+	int disable_count;
 };
 
 static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
@@ -136,6 +141,13 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
 		goto out;
 	}
 
+	/*
+	 * Setting the dpll is asynchronous since clk_set_rate grabs a global
+	 * common clk lock and set_rate for the dpll takes up to one display
+	 * frame to complete. We still need to wait for the set_rate to complete
+	 * here, though, before we change voltage.
+	 */
+	rockchip_ddrclk_wait_set_rate(dmcfreq->dmc_clk);
 	/*
 	 * Check the dpll rate,
 	 * There only two result we will get,
@@ -202,40 +214,15 @@ static struct devfreq_dev_profile rk3399_devfreq_dmc_profile = {
 static __maybe_unused int rk3399_dmcfreq_suspend(struct device *dev)
 {
 	struct rk3399_dmcfreq *dmcfreq = dev_get_drvdata(dev);
-	int ret = 0;
-
-	ret = devfreq_event_disable_edev(dmcfreq->edev);
-	if (ret < 0) {
-		dev_err(dev, "failed to disable the devfreq-event devices\n");
-		return ret;
-	}
 
-	ret = devfreq_suspend_device(dmcfreq->devfreq);
-	if (ret < 0) {
-		dev_err(dev, "failed to suspend the devfreq devices\n");
-		return ret;
-	}
-
-	return 0;
+	return rockchip_dmcfreq_block(dmcfreq->devfreq);
 }
 
 static __maybe_unused int rk3399_dmcfreq_resume(struct device *dev)
 {
 	struct rk3399_dmcfreq *dmcfreq = dev_get_drvdata(dev);
-	int ret = 0;
 
-	ret = devfreq_event_enable_edev(dmcfreq->edev);
-	if (ret < 0) {
-		dev_err(dev, "failed to enable the devfreq-event devices\n");
-		return ret;
-	}
-
-	ret = devfreq_resume_device(dmcfreq->devfreq);
-	if (ret < 0) {
-		dev_err(dev, "failed to resume the devfreq devices\n");
-		return ret;
-	}
-	return ret;
+	return rockchip_dmcfreq_unblock(dmcfreq->devfreq);
 }
 
 static SIMPLE_DEV_PM_OPS(rk3399_dmcfreq_pm, rk3399_dmcfreq_suspend,
@@ -308,6 +295,88 @@ static int of_get_ddr_timings(struct dram_timing *timing,
 	return ret;
 }
 
+int rockchip_dmcfreq_register_clk_sync_nb(struct devfreq *devfreq,
+					struct notifier_block *nb)
+{
+	struct rk3399_dmcfreq *dmcfreq = dev_get_drvdata(devfreq->dev.parent);
+	int ret;
+
+	mutex_lock(&dmcfreq->en_lock);
+	/*
+	 * We have a short amount of time (~1ms or less typically) to run
+	 * dmcfreq after we sync with the notifier, so syncing with more than
+	 * one notifier is not generally possible. Thus, if more than one sync
+	 * notifier is registered, disable dmcfreq.
+	 */
+	if (dmcfreq->num_sync_nb == 1 && dmcfreq->disable_count <= 0)
+		devfreq_suspend_device(devfreq);
+
+	ret = rockchip_ddrclk_register_sync_nb(dmcfreq->dmc_clk, nb);
+	if (ret == 0)
+		dmcfreq->num_sync_nb++;
+	else if (dmcfreq->num_sync_nb == 1 && dmcfreq->disable_count <= 0)
+		devfreq_resume_device(devfreq);
+
+	mutex_unlock(&dmcfreq->en_lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(rockchip_dmcfreq_register_clk_sync_nb);
+
+int rockchip_dmcfreq_unregister_clk_sync_nb(struct devfreq *devfreq,
+					  struct notifier_block *nb)
+{
+	struct rk3399_dmcfreq *dmcfreq = dev_get_drvdata(devfreq->dev.parent);
+	int ret;
+
+	mutex_lock(&dmcfreq->en_lock);
+	ret = rockchip_ddrclk_unregister_sync_nb(dmcfreq->dmc_clk, nb);
+	if (ret == 0) {
+		dmcfreq->num_sync_nb--;
+		if (dmcfreq->num_sync_nb == 1 && dmcfreq->disable_count <= 0)
+			devfreq_resume_device(devfreq);
+	}
+
+	mutex_unlock(&dmcfreq->en_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(rockchip_dmcfreq_unregister_clk_sync_nb);
+
+int rockchip_dmcfreq_block(struct devfreq *devfreq)
+{
+	struct rk3399_dmcfreq *dmcfreq = dev_get_drvdata(devfreq->dev.parent);
+	int ret = 0;
+
+	mutex_lock(&dmcfreq->en_lock);
+	if (dmcfreq->num_sync_nb <= 1 && dmcfreq->disable_count <= 0)
+		ret = devfreq_suspend_device(devfreq);
+
+	if (!ret)
+		dmcfreq->disable_count++;
+	mutex_unlock(&dmcfreq->en_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(rockchip_dmcfreq_block);
+
+int rockchip_dmcfreq_unblock(struct devfreq *devfreq)
+{
+	struct rk3399_dmcfreq *dmcfreq = dev_get_drvdata(devfreq->dev.parent);
+	int ret = 0;
+
+	mutex_lock(&dmcfreq->en_lock);
+	if (dmcfreq->num_sync_nb <= 1 && dmcfreq->disable_count == 1)
+		ret = devfreq_resume_device(devfreq);
+
+	if (!ret)
+		dmcfreq->disable_count--;
+	WARN_ON(dmcfreq->disable_count < 0);
+	mutex_unlock(&dmcfreq->en_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(rockchip_dmcfreq_unblock);
+
 static int rk3399_dmcfreq_probe(struct platform_device *pdev)
 {
 	struct arm_smccc_res res;
@@ -325,6 +394,7 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	mutex_init(&data->lock);
+	mutex_init(&data->en_lock);
 
 	data->vdd_center = devm_regulator_get(dev, "center");
 	if (IS_ERR(data->vdd_center)) {
diff --git a/drivers/devfreq/rk3399_dmc_priv.h b/drivers/devfreq/rk3399_dmc_priv.h
new file mode 100644
index 000000000000..8ac0340a4990
--- /dev/null
+++ b/drivers/devfreq/rk3399_dmc_priv.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2017-2018 Google, Inc.
+ * Author: Derek Basehore <dbasehore@chromium.org>
+ */
+
+#ifndef __RK3399_DMC_PRIV_H
+#define __RK3399_DMC_PRIV_H
+
+void rockchip_ddrclk_wait_set_rate(struct clk *clk);
+int rockchip_ddrclk_register_sync_nb(struct clk *clk,
+				     struct notifier_block *nb);
+int rockchip_ddrclk_unregister_sync_nb(struct clk *clk,
+				       struct notifier_block *nb);
+#endif
diff --git a/include/soc/rockchip/rk3399_dmc.h b/include/soc/rockchip/rk3399_dmc.h
index 031a62607f61..cc36e08f9b22 100644
--- a/include/soc/rockchip/rk3399_dmc.h
+++ b/include/soc/rockchip/rk3399_dmc.h
@@ -8,7 +8,37 @@
 #define __SOC_RK3399_DMC_H
 
 #include <linux/devfreq.h>
+#include <linux/notifier.h>
+
+#define DMC_MIN_SET_RATE_NS	(250 * NSEC_PER_USEC)
+#define DMC_MIN_VBLANK_NS	(DMC_MIN_SET_RATE_NS + 50 * NSEC_PER_USEC)
 
 int rockchip_pm_register_dmcfreq_notifier(struct devfreq *devfreq);
 
+#if IS_ENABLED(CONFIG_ARM_RK3399_DMC_DEVFREQ)
+int rockchip_dmcfreq_register_clk_sync_nb(struct devfreq *devfreq,
+					  struct notifier_block *nb);
+
+int rockchip_dmcfreq_unregister_clk_sync_nb(struct devfreq *devfreq,
+					    struct notifier_block *nb);
+
+int rockchip_dmcfreq_block(struct devfreq *devfreq);
+
+int rockchip_dmcfreq_unblock(struct devfreq *devfreq);
+#else
+static inline int
+rockchip_dmcfreq_register_clk_sync_nb(struct devfreq *devfreq,
+				      struct notifier_block *nb)
+{ return 0; }
+
+static inline int
+rockchip_dmcfreq_unregister_clk_sync_nb(struct devfreq *devfreq,
+					struct notifier_block *nb)
+{ return 0; }
+
+static inline int rockchip_dmcfreq_block(struct devfreq *devfreq) { return 0; }
+
+static inline int rockchip_dmcfreq_unblock(struct devfreq *devfreq)
+{ return 0; }
+#endif
 #endif
-- 
2.17.0

[RFC PATCH 06/10] devfreq: rk3399_dmc / clk: rockchip: Disable DDR clk timeout on suspend.

[Enric Balletbo i Serra]

From: Derek Basehore <dbasehore@chromium.org>

This disables the timeout for the DDR clk when the governor is
suspended or stopped. This makes sure that the suspend frequency is
set. It also makes sure that the userspace governor will be able to
change the frequency without failing.

Signed-off-by: Derek Basehore <dbasehore@chromium.org>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 drivers/clk/rockchip/clk-ddr.c    | 21 ++++++++++++++++++---
 drivers/devfreq/rk3399_dmc.c      | 20 +++++++++++++++-----
 drivers/devfreq/rk3399_dmc_priv.h |  1 +
 3 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/rockchip/clk-ddr.c b/drivers/clk/rockchip/clk-ddr.c
index 707be1bd8910..de00590af167 100644
--- a/drivers/clk/rockchip/clk-ddr.c
+++ b/drivers/clk/rockchip/clk-ddr.c
@@ -32,6 +32,7 @@ struct rockchip_ddrclk {
 	int		div_shift;
 	int		div_width;
 	int		ddr_flag;
+	bool		timeout_en;
 	unsigned long	cached_rate;
 	struct work_struct set_rate_work;
 	struct mutex	lock;
@@ -52,8 +53,9 @@ static void rockchip_ddrclk_set_rate_func(struct work_struct *work)
 
 	mutex_lock(&ddrclk->lock);
 	for (i = 0; i < DDRCLK_SET_RATE_MAX_RETRIES; i++) {
-		ret = raw_notifier_call_chain(&ddrclk->sync_chain, 0, &timeout);
-		if (ret == NOTIFY_BAD)
+		ret = raw_notifier_call_chain(&ddrclk->sync_chain, 0,
+					      &timeout);
+		if (ddrclk->timeout_en && ret == NOTIFY_BAD)
 			goto out;
 
 		/*
@@ -63,7 +65,8 @@ static void rockchip_ddrclk_set_rate_func(struct work_struct *work)
 		 * at the wrong time.
 		 */
 		local_irq_disable();
-		if (ktime_after(ktime_add_ns(ktime_get(), DMC_MIN_VBLANK_NS),
+		if (ddrclk->timeout_en &&
+		    ktime_after(ktime_add_ns(ktime_get(), DMC_MIN_VBLANK_NS),
 				timeout)) {
 			local_irq_enable();
 			continue;
@@ -79,6 +82,17 @@ static void rockchip_ddrclk_set_rate_func(struct work_struct *work)
 	mutex_unlock(&ddrclk->lock);
 }
 
+void rockchip_ddrclk_set_timeout_en(struct clk *clk, bool enable)
+{
+	struct clk_hw *hw = __clk_get_hw(clk);
+	struct rockchip_ddrclk *ddrclk = to_rockchip_ddrclk_hw(hw);
+
+	mutex_lock(&ddrclk->lock);
+	ddrclk->timeout_en = enable;
+	mutex_unlock(&ddrclk->lock);
+}
+EXPORT_SYMBOL(rockchip_ddrclk_set_timeout_en);
+
 int rockchip_ddrclk_register_sync_nb(struct clk *clk, struct notifier_block *nb)
 {
 	struct clk_hw *hw = __clk_get_hw(clk);
@@ -232,6 +246,7 @@ struct clk *rockchip_clk_register_ddrclk(const char *name, int flags,
 	ddrclk->div_shift = div_shift;
 	ddrclk->div_width = div_width;
 	ddrclk->ddr_flag = ddr_flag;
+	ddrclk->timeout_en = true;
 	mutex_init(&ddrclk->lock);
 	INIT_WORK(&ddrclk->set_rate_work, rockchip_ddrclk_set_rate_func);
 	RAW_INIT_NOTIFIER_HEAD(&ddrclk->sync_chain);
diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
index c174d13afe92..2fa4ab653c35 100644
--- a/drivers/devfreq/rk3399_dmc.c
+++ b/drivers/devfreq/rk3399_dmc.c
@@ -308,14 +308,18 @@ int rockchip_dmcfreq_register_clk_sync_nb(struct devfreq *devfreq,
 	 * one notifier is not generally possible. Thus, if more than one sync
 	 * notifier is registered, disable dmcfreq.
 	 */
-	if (dmcfreq->num_sync_nb == 1 && dmcfreq->disable_count <= 0)
+	if (dmcfreq->num_sync_nb == 1 && dmcfreq->disable_count <= 0) {
+		rockchip_ddrclk_set_timeout_en(dmcfreq->dmc_clk, false);
 		devfreq_suspend_device(devfreq);
+	}
 
 	ret = rockchip_ddrclk_register_sync_nb(dmcfreq->dmc_clk, nb);
 	if (ret == 0)
 		dmcfreq->num_sync_nb++;
-	else if (dmcfreq->num_sync_nb == 1 && dmcfreq->disable_count <= 0)
+	else if (dmcfreq->num_sync_nb == 1 && dmcfreq->disable_count <= 0) {
+		rockchip_ddrclk_set_timeout_en(dmcfreq->dmc_clk, true);
 		devfreq_resume_device(devfreq);
+	}
 
 	mutex_unlock(&dmcfreq->en_lock);
 	return ret;
@@ -332,8 +336,10 @@ int rockchip_dmcfreq_unregister_clk_sync_nb(struct devfreq *devfreq,
 	ret = rockchip_ddrclk_unregister_sync_nb(dmcfreq->dmc_clk, nb);
 	if (ret == 0) {
 		dmcfreq->num_sync_nb--;
-		if (dmcfreq->num_sync_nb == 1 && dmcfreq->disable_count <= 0)
+		if (dmcfreq->num_sync_nb == 1 && dmcfreq->disable_count <= 0) {
+			rockchip_ddrclk_set_timeout_en(dmcfreq->dmc_clk, true);
 			devfreq_resume_device(devfreq);
+		}
 	}
 
 	mutex_unlock(&dmcfreq->en_lock);
@@ -348,8 +354,10 @@ int rockchip_dmcfreq_block(struct devfreq *devfreq)
 	int ret = 0;
 
 	mutex_lock(&dmcfreq->en_lock);
-	if (dmcfreq->num_sync_nb <= 1 && dmcfreq->disable_count <= 0)
+	if (dmcfreq->num_sync_nb <= 1 && dmcfreq->disable_count <= 0) {
+		rockchip_ddrclk_set_timeout_en(dmcfreq->dmc_clk, false);
 		ret = devfreq_suspend_device(devfreq);
+	}
 
 	if (!ret)
 		dmcfreq->disable_count++;
@@ -365,8 +373,10 @@ int rockchip_dmcfreq_unblock(struct devfreq *devfreq)
 	int ret = 0;
 
 	mutex_lock(&dmcfreq->en_lock);
-	if (dmcfreq->num_sync_nb <= 1 && dmcfreq->disable_count == 1)
+	if (dmcfreq->num_sync_nb <= 1 && dmcfreq->disable_count == 1) {
+		rockchip_ddrclk_set_timeout_en(dmcfreq->dmc_clk, true);
 		ret = devfreq_resume_device(devfreq);
+	}
 
 	if (!ret)
 		dmcfreq->disable_count--;
diff --git a/drivers/devfreq/rk3399_dmc_priv.h b/drivers/devfreq/rk3399_dmc_priv.h
index 8ac0340a4990..1ad46f4b15cc 100644
--- a/drivers/devfreq/rk3399_dmc_priv.h
+++ b/drivers/devfreq/rk3399_dmc_priv.h
@@ -7,6 +7,7 @@
 #ifndef __RK3399_DMC_PRIV_H
 #define __RK3399_DMC_PRIV_H
 
+void rockchip_ddrclk_set_timeout_en(struct clk *clk, bool enable);
 void rockchip_ddrclk_wait_set_rate(struct clk *clk);
 int rockchip_ddrclk_register_sync_nb(struct clk *clk,
 				     struct notifier_block *nb);
-- 
2.17.0

[RFC PATCH 07/10] clk: rockchip: set clk-ddr to GET_RATE_NOCACHE.

[Enric Balletbo i Serra]

From: Derek Basehore <dbasehore@chromium.org>

This adds the flag to the clk-ddr in rockchip to not use the cached
rate for get_rate. This is to handle timeout error conditions in SMC
for the set rate function.

Signed-off-by: Derek Basehore <dbasehore@chromium.org>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 drivers/clk/rockchip/clk-ddr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/rockchip/clk-ddr.c b/drivers/clk/rockchip/clk-ddr.c
index de00590af167..dffd5ac5847b 100644
--- a/drivers/clk/rockchip/clk-ddr.c
+++ b/drivers/clk/rockchip/clk-ddr.c
@@ -226,7 +226,7 @@ struct clk *rockchip_clk_register_ddrclk(const char *name, int flags,
 	init.num_parents = num_parents;
 
 	init.flags = flags;
-	init.flags |= CLK_SET_RATE_NO_REPARENT;
+	init.flags |= CLK_SET_RATE_NO_REPARENT | CLK_GET_RATE_NOCACHE;
 
 	switch (ddr_flag) {
 	case ROCKCHIP_DDRCLK_SIP:
-- 
2.17.0

[RFC PATCH 08/10] drm: rockchip: Add DDR devfreq support.

[Enric Balletbo i Serra]

From: Sean Paul <seanpaul@chromium.org>

Add support for devfreq to dynamically control the DDR frequency. It
will activate when there is one CRTC active, and disable if more than
one becomes active (to avoid flickering on one of the screens).

Signed-off-by: Sean Paul <seanpaul@chromium.org>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 46 +++++++++++++++++++++
 drivers/gpu/drm/rockchip/rockchip_drm_drv.h |  9 ++++
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c  | 35 ++++++++++++++++
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 34 +++++++++++++++
 4 files changed, 124 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index f814d37b1db2..e774f3c7c325 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -19,6 +19,8 @@
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_of.h>
+#include <linux/devfreq.h>
+#include <linux/devfreq-event.h>
 #include <linux/dma-mapping.h>
 #include <linux/dma-iommu.h>
 #include <linux/pm_runtime.h>
@@ -77,6 +79,46 @@ void rockchip_drm_dma_detach_device(struct drm_device *drm_dev,
 	iommu_detach_device(domain, dev);
 }
 
+#if IS_ENABLED(CONFIG_ARM_RK3399_DMC_DEVFREQ)
+static int rockchip_drm_init_devfreq(struct device *dev,
+				     struct rockchip_drm_private *priv)
+{
+	struct devfreq *devfreq;
+	struct devfreq_event_dev *edev;
+	int ret;
+
+	devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
+	if (IS_ERR(devfreq)) {
+		ret = PTR_ERR(devfreq);
+		if (ret == -ENODEV) {
+			DRM_DEV_INFO(dev, "devfreq missing, skip\n");
+			return 0;
+		}
+		return ret;
+	}
+
+	edev = devfreq_event_get_edev_by_phandle(devfreq->dev.parent, 0);
+	if (IS_ERR(edev)) {
+		ret = PTR_ERR(edev);
+		if (ret == -ENODEV) {
+			DRM_DEV_INFO(dev, "devfreq edev missing, skip\n");
+			return 0;
+		}
+		return ret;
+	}
+
+	priv->devfreq = devfreq;
+	priv->devfreq_event_dev = edev;
+	return 0;
+}
+#else
+static int rockchip_drm_init_devfreq(struct device *dev,
+				     struct rockchip_drm_private *priv)
+{
+	return 0;
+}
+#endif
+
 static int rockchip_drm_init_iommu(struct drm_device *drm_dev)
 {
 	struct rockchip_drm_private *private = drm_dev->dev_private;
@@ -136,6 +178,10 @@ static int rockchip_drm_bind(struct device *dev)
 	INIT_LIST_HEAD(&private->psr_list);
 	mutex_init(&private->psr_list_lock);
 
+	ret = rockchip_drm_init_devfreq(dev, private);
+	if (ret)
+		goto err_free;
+
 	ret = rockchip_drm_init_iommu(drm_dev);
 	if (ret)
 		goto err_free;
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
index 3a6ebfc26036..cc13fac59644 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
@@ -57,6 +57,10 @@ struct rockchip_drm_private {
 	struct drm_mm mm;
 	struct list_head psr_list;
 	struct mutex psr_list_lock;
+
+	struct devfreq *devfreq;
+	struct devfreq_event_dev *devfreq_event_dev;
+	bool dmc_disable_flag;
 };
 
 int rockchip_drm_dma_attach_device(struct drm_device *drm_dev,
@@ -65,6 +69,11 @@ void rockchip_drm_dma_detach_device(struct drm_device *drm_dev,
 				    struct device *dev);
 int rockchip_drm_wait_vact_end(struct drm_crtc *crtc, unsigned int mstimeout);
 
+uint32_t rockchip_drm_get_vblank_ns(struct drm_display_mode *mode);
+
+void rockchip_drm_enable_dmc(struct rockchip_drm_private *priv);
+void rockchip_drm_disable_dmc(struct rockchip_drm_private *priv);
+
 extern struct platform_driver cdn_dp_driver;
 extern struct platform_driver dw_hdmi_rockchip_pltfm_driver;
 extern struct platform_driver dw_mipi_dsi_driver;
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
index d4f4118b482d..bf10f7982278 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
@@ -18,6 +18,7 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_crtc_helper.h>
+#include <soc/rockchip/rk3399_dmc.h>
 
 #include "rockchip_drm_drv.h"
 #include "rockchip_drm_fb.h"
@@ -185,6 +186,16 @@ rockchip_drm_psr_inhibit_get_state(struct drm_atomic_state *state)
 		rockchip_drm_psr_inhibit_get(encoder);
 }
 
+uint32_t rockchip_drm_get_vblank_ns(struct drm_display_mode *mode)
+{
+	uint64_t vblank_time = mode->vtotal - mode->vdisplay;
+
+	vblank_time *= (uint64_t)NSEC_PER_SEC * mode->htotal;
+	do_div(vblank_time, mode->clock * 1000);
+
+	return vblank_time;
+}
+
 static void
 rockchip_drm_psr_inhibit_put_state(struct drm_atomic_state *state)
 {
@@ -207,9 +218,28 @@ static void
 rockchip_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state)
 {
 	struct drm_device *dev = old_state->dev;
+	struct rockchip_drm_private *priv = dev->dev_private;
+	struct drm_crtc *crtc;
+	struct drm_display_mode *mode;
+	bool force_dmc_off = false;
+
+	drm_for_each_crtc(crtc, dev) {
+		if (crtc->state->active) {
+			mode = &crtc->state->adjusted_mode;
+			if (rockchip_drm_get_vblank_ns(mode) <
+			    DMC_MIN_VBLANK_NS)
+				force_dmc_off = true;
+		}
+	}
 
 	rockchip_drm_psr_inhibit_get_state(old_state);
 
+	/* If disabling dmc, disable it before committing mode set changes. */
+	if (force_dmc_off && !priv->dmc_disable_flag) {
+		rockchip_dmcfreq_block(priv->devfreq);
+		priv->dmc_disable_flag = true;
+	}
+
 	drm_atomic_helper_commit_modeset_disables(dev, old_state);
 
 	drm_atomic_helper_commit_modeset_enables(dev, old_state);
@@ -224,6 +254,11 @@ rockchip_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state)
 	drm_atomic_helper_wait_for_vblanks(dev, old_state);
 
 	drm_atomic_helper_cleanup_planes(dev, old_state);
+
+	if (!force_dmc_off && priv->dmc_disable_flag) {
+		rockchip_dmcfreq_unblock(priv->devfreq);
+		priv->dmc_disable_flag = false;
+	}
 }
 
 static const struct drm_mode_config_helper_funcs rockchip_mode_config_helpers = {
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index f14a10ca4792..32f052ee15b9 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -36,6 +36,8 @@
 #include <linux/reset.h>
 #include <linux/delay.h>
 
+#include <soc/rockchip/rk3399_dmc.h>
+
 #include "rockchip_drm_drv.h"
 #include "rockchip_drm_gem.h"
 #include "rockchip_drm_fb.h"
@@ -103,7 +105,9 @@ struct vop {
 	struct drm_flip_work fb_unref_work;
 	unsigned long pending;
 
+	ktime_t line_flag_timestamp;
 	struct completion line_flag_completion;
+	struct notifier_block dmc_nb;
 
 	const struct vop_data *data;
 
@@ -569,10 +573,12 @@ static int vop_enable(struct drm_crtc *crtc)
 static void vop_crtc_atomic_disable(struct drm_crtc *crtc,
 				    struct drm_crtc_state *old_state)
 {
+	struct rockchip_drm_private *priv = crtc->dev->dev_private;
 	struct vop *vop = to_vop(crtc);
 
 	WARN_ON(vop->event);
 
+	rockchip_dmcfreq_unregister_clk_sync_nb(priv->devfreq, &vop->dmc_nb);
 	mutex_lock(&vop->vop_lock);
 	drm_crtc_vblank_off(crtc);
 
@@ -861,6 +867,7 @@ static void vop_crtc_atomic_enable(struct drm_crtc *crtc,
 	const struct vop_data *vop_data = vop->data;
 	struct rockchip_crtc_state *s = to_rockchip_crtc_state(crtc->state);
 	struct drm_display_mode *adjusted_mode = &crtc->state->adjusted_mode;
+	struct rockchip_drm_private *priv = crtc->dev->dev_private;
 	u16 hsync_len = adjusted_mode->hsync_end - adjusted_mode->hsync_start;
 	u16 hdisplay = adjusted_mode->hdisplay;
 	u16 htotal = adjusted_mode->htotal;
@@ -951,6 +958,31 @@ static void vop_crtc_atomic_enable(struct drm_crtc *crtc,
 
 	VOP_REG_SET(vop, common, standby, 0);
 	mutex_unlock(&vop->vop_lock);
+	rockchip_dmcfreq_register_clk_sync_nb(priv->devfreq, &vop->dmc_nb);
+}
+
+static int dmc_notify(struct notifier_block *nb,
+		      unsigned long action,
+		      void *data)
+{
+	struct vop *vop = container_of(nb, struct vop, dmc_nb);
+	struct drm_crtc *crtc = &vop->crtc;
+	ktime_t *timeout = data;
+	int ret;
+
+	if (WARN_ON(!vop->is_enabled))
+		return NOTIFY_BAD;
+
+	ret = rockchip_drm_wait_vact_end(crtc, 100);
+	*timeout = ktime_add_ns(vop->line_flag_timestamp,
+				rockchip_drm_get_vblank_ns(&crtc->mode));
+	if (ret) {
+		dev_err(vop->dev, "%s: Line flag interrupt did not arrive\n",
+			__func__);
+		return NOTIFY_BAD;
+	}
+
+	return NOTIFY_STOP;
 }
 
 static bool vop_fs_irq_is_pending(struct vop *vop)
@@ -1190,6 +1222,7 @@ static irqreturn_t vop_isr(int irq, void *data)
 	}
 
 	if (active_irqs & LINE_FLAG_INTR) {
+		vop->line_flag_timestamp = ktime_get();
 		complete(&vop->line_flag_completion);
 		active_irqs &= ~LINE_FLAG_INTR;
 		ret = IRQ_HANDLED;
@@ -1566,6 +1599,7 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
 	spin_lock_init(&vop->reg_lock);
 	spin_lock_init(&vop->irq_lock);
 	mutex_init(&vop->vop_lock);
+	vop->dmc_nb.notifier_call = dmc_notify;
 
 	ret = vop_create_crtc(vop);
 	if (ret)
-- 
2.17.0

[RFC PATCH 09/10] arm64: dts: rk3399: Add dfi and dmc nodes.

[Enric Balletbo i Serra]

From: Lin Huang <hl@rock-chips.com>

These are required to support DDR DVFS on rk3399 platform. The patch also
introduces two new files (rk3399-dram.h and rk3399-dram-default-timing)
with default DRAM settings.

Signed-off-by: Lin Huang <hl@rock-chips.com>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 .../rockchip/rk3399-dram-default-timing.dtsi  | 38 ++++++++++
 arch/arm64/boot/dts/rockchip/rk3399-dram.h    | 73 +++++++++++++++++++
 .../boot/dts/rockchip/rk3399-op1-opp.dtsi     | 29 ++++++++
 arch/arm64/boot/dts/rockchip/rk3399.dtsi      | 20 +++++
 4 files changed, 160 insertions(+)
 create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-dram-default-timing.dtsi
 create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-dram.h

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-dram-default-timing.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-dram-default-timing.dtsi
new file mode 100644
index 000000000000..4dfe3e1d8bdf
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3399-dram-default-timing.dtsi
@@ -0,0 +1,38 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR X11)
+/*
+ * Copyright (c) 2016-2018, Fuzhou Rockchip Electronics Co., Ltd
+ *
+ * Author: Lin Huang <hl@rock-chips.com>
+ */
+
+#include "rk3399-dram.h"
+
+rockchip,ddr3_speed_bin = <21>;
+rockchip,pd_idle = <0x40>;
+rockchip,sr_idle = <0x2>;
+rockchip,sr_mc_gate_idle = <0x3>;
+rockchip,srpd_lite_idle	= <0x4>;
+rockchip,standby_idle = <0x2000>;
+rockchip,dram_dll_dis_freq = <300000000>;
+rockchip,phy_dll_dis_freq = <125000000>;
+rockchip,auto_pd_dis_freq = <666000000>;
+rockchip,ddr3_odt_dis_freq = <333000000>;
+rockchip,ddr3_drv = <DDR3_DS_40ohm>;
+rockchip,ddr3_odt = <DDR3_ODT_120ohm>;
+rockchip,phy_ddr3_ca_drv = <PHY_DRV_ODT_40>;
+rockchip,phy_ddr3_dq_drv = <PHY_DRV_ODT_40>;
+rockchip,phy_ddr3_odt = <PHY_DRV_ODT_240>;
+rockchip,lpddr3_odt_dis_freq = <333000000>;
+rockchip,lpddr3_drv = <LP3_DS_34ohm>;
+rockchip,lpddr3_odt = <LP3_ODT_240ohm>;
+rockchip,phy_lpddr3_ca_drv = <PHY_DRV_ODT_40>;
+rockchip,phy_lpddr3_dq_drv = <PHY_DRV_ODT_40>;
+rockchip,phy_lpddr3_odt = <PHY_DRV_ODT_240>;
+rockchip,lpddr4_odt_dis_freq = <333000000>;
+rockchip,lpddr4_drv = <LP4_PDDS_60ohm>;
+rockchip,lpddr4_dq_odt = <LP4_DQ_ODT_40ohm>;
+rockchip,lpddr4_ca_odt = <LP4_CA_ODT_40ohm>;
+rockchip,phy_lpddr4_ca_drv = <PHY_DRV_ODT_40>;
+rockchip,phy_lpddr4_ck_cs_drv = <PHY_DRV_ODT_80>;
+rockchip,phy_lpddr4_dq_drv = <PHY_DRV_ODT_80>;
+rockchip,phy_lpddr4_odt = <PHY_DRV_ODT_60>;
diff --git a/arch/arm64/boot/dts/rockchip/rk3399-dram.h b/arch/arm64/boot/dts/rockchip/rk3399-dram.h
new file mode 100644
index 000000000000..4b3d4a79923b
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3399-dram.h
@@ -0,0 +1,73 @@
+/* SPDX-License-Identifier: (GPL-2.0+ OR X11) */
+/*
+ * Copyright (c) 2016-2018, Fuzhou Rockchip Electronics Co., Ltd
+ *
+ * Author: Lin Huang <hl@rock-chips.com>
+ */
+
+#ifndef _DTS_DRAM_ROCKCHIP_RK3399_H
+#define _DTS_DRAM_ROCKCHIP_RK3399_H
+
+#define DDR3_DS_34ohm		34
+#define DDR3_DS_40ohm		40
+
+#define DDR3_ODT_DIS		0
+#define DDR3_ODT_40ohm		40
+#define DDR3_ODT_60ohm		60
+#define DDR3_ODT_120ohm		120
+
+#define LP2_DS_34ohm		34
+#define LP2_DS_40ohm		40
+#define LP2_DS_48ohm		48
+#define LP2_DS_60ohm		60
+#define LP2_DS_68_6ohm		68	/* optional */
+#define LP2_DS_80ohm		80
+#define LP2_DS_120ohm		120	/* optional */
+
+#define LP3_DS_34ohm		34
+#define LP3_DS_40ohm		40
+#define LP3_DS_48ohm		48
+#define LP3_DS_60ohm		60
+#define LP3_DS_80ohm		80
+#define LP3_DS_34D_40U		3440
+#define LP3_DS_40D_48U		4048
+#define LP3_DS_34D_48U		3448
+
+#define LP3_ODT_DIS		0
+#define LP3_ODT_60ohm		60
+#define LP3_ODT_120ohm		120
+#define LP3_ODT_240ohm		240
+
+#define LP4_PDDS_40ohm		40
+#define LP4_PDDS_48ohm		48
+#define LP4_PDDS_60ohm		60
+#define LP4_PDDS_80ohm		80
+#define LP4_PDDS_120ohm		120
+#define LP4_PDDS_240ohm		240
+
+#define LP4_DQ_ODT_40ohm	40
+#define LP4_DQ_ODT_48ohm	48
+#define LP4_DQ_ODT_60ohm	60
+#define LP4_DQ_ODT_80ohm	80
+#define LP4_DQ_ODT_120ohm	120
+#define LP4_DQ_ODT_240ohm	240
+#define LP4_DQ_ODT_DIS		0
+
+#define LP4_CA_ODT_40ohm	40
+#define LP4_CA_ODT_48ohm	48
+#define LP4_CA_ODT_60ohm	60
+#define LP4_CA_ODT_80ohm	80
+#define LP4_CA_ODT_120ohm	120
+#define LP4_CA_ODT_240ohm	240
+#define LP4_CA_ODT_DIS		0
+
+#define PHY_DRV_ODT_Hi_Z	0
+#define PHY_DRV_ODT_240		240
+#define PHY_DRV_ODT_120		120
+#define PHY_DRV_ODT_80		80
+#define PHY_DRV_ODT_60		60
+#define PHY_DRV_ODT_48		48
+#define PHY_DRV_ODT_40		40
+#define PHY_DRV_ODT_34_3	34
+
+#endif /* _DTS_DRAM_ROCKCHIP_RK3399_H */
diff --git a/arch/arm64/boot/dts/rockchip/rk3399-op1-opp.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-op1-opp.dtsi
index d8a120f945c8..4c634e58425d 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-op1-opp.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-op1-opp.dtsi
@@ -147,6 +147,31 @@
 			opp-microvolt = <1075000>;
 		};
 	};
+
+	dmc_opp_table: dmc_opp_table {
+		compatible = "operating-points-v2";
+
+		opp00 {
+			opp-hz = /bits/ 64 <200000000>;
+			opp-microvolt = <900000>;
+		};
+		opp01 {
+			opp-hz = /bits/ 64 <400000000>;
+			opp-microvolt = <900000>;
+		};
+		opp02 {
+			opp-hz = /bits/ 64 <666000000>;
+			opp-microvolt = <900000>;
+		};
+		opp03 {
+			opp-hz = /bits/ 64 <800000000>;
+			opp-microvolt = <900000>;
+		};
+		opp04 {
+			opp-hz = /bits/ 64 <928000000>;
+			opp-microvolt = <900000>;
+		};
+	};
 };
 
 &cpu_l0 {
@@ -176,3 +201,7 @@
 &gpu {
 	operating-points-v2 = <&gpu_opp_table>;
 };
+
+&dmc {
+	operating-points-v2 = <&dmc_opp_table>;
+};
diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
index 4550c0f82be9..e012cc8ae3d4 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
@@ -1830,6 +1830,26 @@
 		status = "disabled";
 	};
 
+	dfi: dfi@ff630000 {
+		reg = <0x00 0xff630000 0x00 0x4000>;
+		compatible = "rockchip,rk3399-dfi";
+		rockchip,pmu = <&pmugrf>;
+		interrupts = <GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH 0>;
+		clocks = <&cru PCLK_DDR_MON>;
+		clock-names = "pclk_ddr_mon";
+		status = "disabled";
+	};
+
+	dmc: dmc {
+		compatible = "rockchip,rk3399-dmc";
+		rockchip,pmu = <&pmugrf>;
+		devfreq-events = <&dfi>;
+		clocks = <&cru SCLK_DDRC>;
+		clock-names = "dmc_clk";
+		#include "rk3399-dram-default-timing.dtsi"
+		status = "disabled";
+	};
+
 	pinctrl: pinctrl {
 		compatible = "rockchip,rk3399-pinctrl";
 		rockchip,grf = <&grf>;
-- 
2.17.0

[RFC PATCH 10/10] arm64: dts: rockchip: Enable dmc and dfi nodes on gru.

[Enric Balletbo i Serra]

From: Lin Huang <hl@rock-chips.com>

Enable the DMC (Dynamic Memory Controller) and the DFI (DDR PHY Interface)
nodes on gru/kevin boards so we can support DDR DVFS.

Signed-off-by: Lin Huang <hl@rock-chips.com>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi | 21 ++++++++++++++++++++
 arch/arm64/boot/dts/rockchip/rk3399.dtsi     |  2 +-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
index 18f546f2dfd1..ba2cfdd082b8 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
@@ -525,6 +525,12 @@
 	extcon = <&usbc_extcon0>, <&usbc_extcon1>;
 };
 
+&dmc_opp_table {
+	opp04 {
+		opp-suspend;
+	};
+};
+
 /*
  * Set some suspend operating points to avoid OVP in suspend
  *
@@ -600,6 +606,10 @@
 		<400000000>;
 };
 
+&display_subsystem {
+	devfreq = <&dmc>;
+};
+
 &emmc_phy {
 	status = "okay";
 };
@@ -766,6 +776,17 @@ ap_i2c_audio: &i2c8 {
 	status = "okay";
 };
 
+&dfi {
+	status = "okay";
+};
+
+&dmc {
+	status = "okay";
+	center-supply = <&ppvar_centerlogic>;
+	upthreshold = <25>;
+	downdifferential = <15>;
+};
+
 &sdhci {
 	/*
 	 * Signal integrity isn't great at 200 MHz and 150 MHz (DDR) gives the
diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
index e012cc8ae3d4..518c742293fc 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
@@ -160,7 +160,7 @@
 		};
 	};
 
-	display-subsystem {
+	display_subsystem: display-subsystem {
 		compatible = "rockchip,display-subsystem";
 		ports = <&vopl_out>, <&vopb_out>;
 	};
-- 
2.17.0

Re: [RFC PATCH 01/10] devfreq: rockchip-dfi: Move GRF definitions to a common place.

[Chanwoo Choi]

Hi Enric,

On 2018년 05월 15일 06:16, Enric Balletbo i Serra wrote:
> Some rk3399 GRF (Generic Register Files) definitions can be used for
> different drivers. Move these definitions to a common include so we
> don't need to duplicate these definitions.
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> 
>  drivers/devfreq/event/rockchip-dfi.c | 23 +++++++----------------
>  include/soc/rockchip/rk3399_grf.h    | 21 +++++++++++++++++++++
>  2 files changed, 28 insertions(+), 16 deletions(-)
>  create mode 100644 include/soc/rockchip/rk3399_grf.h
> 
> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> index 22b113363ffc..2fbbcbeb644f 100644
> --- a/drivers/devfreq/event/rockchip-dfi.c
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -26,6 +26,8 @@
>  #include <linux/list.h>
>  #include <linux/of.h>
>  
> +#include <soc/rockchip/rk3399_grf.h>
> +
>  #define RK3399_DMC_NUM_CH	2
>  
>  /* DDRMON_CTRL */
> @@ -43,18 +45,6 @@
>  #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;
> @@ -83,16 +73,17 @@ static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
>  	u32 ddr_type;
>  
>  	/* get ddr type */
> -	regmap_read(info->regmap_pmu, PMUGRF_OS_REG2, &val);
> -	ddr_type = (val >> DDRTYPE_SHIFT) & DDRTYPE_MASK;
> +	regmap_read(info->regmap_pmu, RK3399_PMUGRF_OS_REG2, &val);
> +	ddr_type = (val >> RK3399_PMUGRF_DDRTYPE_SHIFT) &
> +		    RK3399_PMUGRF_DDRTYPE_MASK;
>  
>  	/* clear DDRMON_CTRL setting */
>  	writel_relaxed(CLR_DDRMON_CTRL, dfi_regs + DDRMON_CTRL);
>  
>  	/* set ddr type to dfi */
> -	if (ddr_type == LPDDR3)
> +	if (ddr_type == RK3399_PMUGRF_DDRTYPE_LPDDR3)
>  		writel_relaxed(LPDDR3_EN, dfi_regs + DDRMON_CTRL);
> -	else if (ddr_type == LPDDR4)
> +	else if (ddr_type == RK3399_PMUGRF_DDRTYPE_LPDDR4)
>  		writel_relaxed(LPDDR4_EN, dfi_regs + DDRMON_CTRL);
>  
>  	/* enable count, use software mode */
> diff --git a/include/soc/rockchip/rk3399_grf.h b/include/soc/rockchip/rk3399_grf.h
> new file mode 100644
> index 000000000000..0f94034e2e9a
> --- /dev/null
> +++ b/include/soc/rockchip/rk3399_grf.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Rockchip Generic Register Files definitions
> + *
> + * Copyright (c) 2018, Collabora Ltd.
> + * Author: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> + */
> +
> +#ifndef __SOC_RK3399_GRF_H
> +#define __SOC_RK3399_GRF_H
> +
> +/* PMU GRF Registers */
> +#define RK3399_PMUGRF_OS_REG2		0x308
> +#define RK3399_PMUGRF_DDRTYPE_SHIFT	13
> +#define RK3399_PMUGRF_DDRTYPE_MASK	7
> +#define RK3399_PMUGRF_DDRTYPE_DDR3	3
> +#define RK3399_PMUGRF_DDRTYPE_LPDDR2	5
> +#define RK3399_PMUGRF_DDRTYPE_LPDDR3	6
> +#define RK3399_PMUGRF_DDRTYPE_LPDDR4	7
> +
> +#endif
> 

Looks good to me.
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [RFC PATCH 03/10] devfreq: rk3399_dmc: Pass ODT and auto power down parameters to TF-A.

[Chanwoo Choi]

Hi,

On 2018년 05월 15일 06:16, Enric Balletbo i Serra wrote:
> Trusted Firmware-A (TF-A) for rk3399 implements a SiP call to get the
> on-die termination (ODT) and auto power down parameters from kernel,
> this patch adds the functionality to do this. Also, if DDR clock
> frequency is lower than the on-die termination (ODT) disable frequency
> this driver should disable the DDR ODT.

I have a question.
'disable frequency' is the same meaning of 'disable the DDR ODT'?

> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> 
>  drivers/devfreq/rk3399_dmc.c        | 50 ++++++++++++++++++++++++++++-
>  include/soc/rockchip/rockchip_sip.h |  1 +
>  2 files changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
> index d5c03e5abe13..cc1bbca3fb15 100644
> --- a/drivers/devfreq/rk3399_dmc.c
> +++ b/drivers/devfreq/rk3399_dmc.c
> @@ -18,14 +18,17 @@
>  #include <linux/devfreq.h>
>  #include <linux/devfreq-event.h>
>  #include <linux/interrupt.h>
> +#include <linux/mfd/syscon.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_opp.h>
> +#include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/rwsem.h>
>  #include <linux/suspend.h>
>  
> +#include <soc/rockchip/rk3399_grf.h>
>  #include <soc/rockchip/rockchip_sip.h>
>  
>  struct dram_timing {
> @@ -69,8 +72,11 @@ struct rk3399_dmcfreq {
>  	struct mutex lock;
>  	struct dram_timing timing;
>  	struct regulator *vdd_center;
> +	struct regmap *regmap_pmu;
>  	unsigned long rate, target_rate;
>  	unsigned long volt, target_volt;
> +	unsigned int odt_dis_freq;
> +	int odt_pd_arg0, odt_pd_arg1;
>  };
>  
>  static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
> @@ -80,6 +86,8 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
>  	struct dev_pm_opp *opp;
>  	unsigned long old_clk_rate = dmcfreq->rate;
>  	unsigned long target_volt, target_rate;
> +	struct arm_smccc_res res;
> +	int dram_flag;
>  	int err;
>  
>  	opp = devfreq_recommended_opp(dev, freq, flags);
> @@ -95,6 +103,15 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
>  
>  	mutex_lock(&dmcfreq->lock);
>  
> +	dram_flag = 0;

Also, if dram_flag is 0, it mean that disable ODT frequency?
If it's right, you better to define the precise variables as following
instead of just integer(0 or 1).
For example,
- ROCKCHIP_SIP_DRAM_FREQ_ENABLE
- ROCKCHIP_SIP_DRAM_FREQ_DISABLE

> +	if (target_rate >= dmcfreq->odt_dis_freq)
> +		dram_flag = 1;
> +
> +	arm_smccc_smc(ROCKCHIP_SIP_DRAM_FREQ, dmcfreq->odt_pd_arg0,
> +		      dmcfreq->odt_pd_arg1,
> +		      ROCKCHIP_SIP_CONFIG_DRAM_SET_ODT_PD,
> +		      dram_flag, 0, 0, 0, &res);
> +

This operation is special for only rk3399_dmc. It is difficult
to understand what to do. I recommend you better to add the detailed comment
with code.

>  	/*
>  	 * If frequency scaling from low to high, adjust voltage first.
>  	 * If frequency scaling from high to low, adjust frequency first.
> @@ -294,11 +311,13 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
>  {
>  	struct arm_smccc_res res;
>  	struct device *dev = &pdev->dev;
> -	struct device_node *np = pdev->dev.of_node;
> +	struct device_node *np = pdev->dev.of_node, *node;
>  	struct rk3399_dmcfreq *data;
>  	int ret, index, size;
>  	uint32_t *timing;
>  	struct dev_pm_opp *opp;
> +	u32 ddr_type;
> +	u32 val;
>  
>  	data = devm_kzalloc(dev, sizeof(struct rk3399_dmcfreq), GFP_KERNEL);
>  	if (!data)
> @@ -334,6 +353,29 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	/* 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);
> +	}
> +
> +	/* Get DDR type */
> +	regmap_read(data->regmap_pmu, RK3399_PMUGRF_OS_REG2, &val);
> +	ddr_type = (val >> RK3399_PMUGRF_DDRTYPE_SHIFT) &
> +		    RK3399_PMUGRF_DDRTYPE_MASK;
> +
> +	/* Get the odt_dis_freq parameter in function of the DDR type */
> +	if (ddr_type == RK3399_PMUGRF_DDRTYPE_DDR3)
> +		data->odt_dis_freq = data->timing.ddr3_odt_dis_freq;
> +	else if (ddr_type == RK3399_PMUGRF_DDRTYPE_LPDDR3)
> +		data->odt_dis_freq = data->timing.lpddr3_odt_dis_freq;
> +	else if (ddr_type == RK3399_PMUGRF_DDRTYPE_LPDDR4)
> +		data->odt_dis_freq = data->timing.lpddr4_odt_dis_freq;
> +	else
> +		return -EINVAL;
> +

how about using 'switch' statement?

>  	/*
>  	 * Get dram timing and pass it to arm trust firmware,
>  	 * the dram drvier in arm trust firmware will get these
> @@ -358,6 +400,12 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
>  		      ROCKCHIP_SIP_CONFIG_DRAM_INIT,
>  		      0, 0, 0, 0, &res);
>  
> +	data->odt_pd_arg0 = (data->timing.sr_idle & 0xff) |
> +			    ((data->timing.sr_mc_gate_idle & 0xff) << 8) |
> +			    ((data->timing.standby_idle & 0xffff) << 16);
> +	data->odt_pd_arg1 = (data->timing.pd_idle & 0xfff) |
> +			    ((data->timing.srpd_lite_idle & 0xfff) << 16);
> +

odt_pd_arg0 and odt_pd_arg1 might be used for disabling/enabling the ODT frequency.
As I commented, it depend on only rk3399_dmc. You better to add detailed comment.

And I prefer to define the XXX_SHIFT/XXX_MASK definition instead of
using 8/16/0xff/0xffff for the readability.

>  	/*
>  	 * We add a devfreq driver to our parent since it has a device tree node
>  	 * with operating points.
> diff --git a/include/soc/rockchip/rockchip_sip.h b/include/soc/rockchip/rockchip_sip.h
> index 7e28092c4d3d..ad9482c56797 100644
> --- a/include/soc/rockchip/rockchip_sip.h
> +++ b/include/soc/rockchip/rockchip_sip.h
> @@ -23,5 +23,6 @@
>  #define ROCKCHIP_SIP_CONFIG_DRAM_GET_RATE	0x05
>  #define ROCKCHIP_SIP_CONFIG_DRAM_CLR_IRQ	0x06
>  #define ROCKCHIP_SIP_CONFIG_DRAM_SET_PARAM	0x07
> +#define ROCKCHIP_SIP_CONFIG_DRAM_SET_ODT_PD	0x08
>  
>  #endif
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [RFC PATCH 02/10] dt-bindings: devfreq: rk3399_dmc: Add rockchip,pmu phandle.

[Chanwoo Choi]

Hi,

On 2018년 05월 15일 06:16, Enric Balletbo i Serra wrote:
> The Rockchip DMC (Dynamic Memory Interface) needs to access to the PMU
> general register files to know the DRAM type, so add a phandle to the
> syscon that manages these registers.
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> 
>  Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt b/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
> index 0ec68141f85a..951789c0cdd6 100644
> --- a/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
> +++ b/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt
> @@ -12,6 +12,8 @@ Required properties:
>  			 for details.
>  - center-supply:	 DMC supply node.
>  - status:		 Marks the node enabled/disabled.
> +- rockchip,pmu:		 Phandle to the syscon managing the "PMU general register
> +			 files".
>  
>  Optional properties:
>  - interrupts:		 The CPU interrupt number. The interrupt specifier
> 

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

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [RFC PATCH 01/10] devfreq: rockchip-dfi: Move GRF definitions to a common place.

[Robin Murphy]

Hi Enric,

On 14/05/18 22:16, Enric Balletbo i Serra wrote:
> Some rk3399 GRF (Generic Register Files) definitions can be used for
> different drivers. Move these definitions to a common include so we
> don't need to duplicate these definitions.
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> 
>   drivers/devfreq/event/rockchip-dfi.c | 23 +++++++----------------
>   include/soc/rockchip/rk3399_grf.h    | 21 +++++++++++++++++++++
>   2 files changed, 28 insertions(+), 16 deletions(-)
>   create mode 100644 include/soc/rockchip/rk3399_grf.h
> 
> diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> index 22b113363ffc..2fbbcbeb644f 100644
> --- a/drivers/devfreq/event/rockchip-dfi.c
> +++ b/drivers/devfreq/event/rockchip-dfi.c
> @@ -26,6 +26,8 @@
>   #include <linux/list.h>
>   #include <linux/of.h>
>   
> +#include <soc/rockchip/rk3399_grf.h>
> +
>   #define RK3399_DMC_NUM_CH	2
>   
>   /* DDRMON_CTRL */
> @@ -43,18 +45,6 @@
>   #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;
> @@ -83,16 +73,17 @@ static void rockchip_dfi_start_hardware_counter(struct devfreq_event_dev *edev)
>   	u32 ddr_type;
>   
>   	/* get ddr type */
> -	regmap_read(info->regmap_pmu, PMUGRF_OS_REG2, &val);
> -	ddr_type = (val >> DDRTYPE_SHIFT) & DDRTYPE_MASK;
> +	regmap_read(info->regmap_pmu, RK3399_PMUGRF_OS_REG2, &val);
> +	ddr_type = (val >> RK3399_PMUGRF_DDRTYPE_SHIFT) &
> +		    RK3399_PMUGRF_DDRTYPE_MASK;
>   
>   	/* clear DDRMON_CTRL setting */
>   	writel_relaxed(CLR_DDRMON_CTRL, dfi_regs + DDRMON_CTRL);
>   
>   	/* set ddr type to dfi */
> -	if (ddr_type == LPDDR3)
> +	if (ddr_type == RK3399_PMUGRF_DDRTYPE_LPDDR3)
>   		writel_relaxed(LPDDR3_EN, dfi_regs + DDRMON_CTRL);
> -	else if (ddr_type == LPDDR4)
> +	else if (ddr_type == RK3399_PMUGRF_DDRTYPE_LPDDR4)
>   		writel_relaxed(LPDDR4_EN, dfi_regs + DDRMON_CTRL);
>   
>   	/* enable count, use software mode */
> diff --git a/include/soc/rockchip/rk3399_grf.h b/include/soc/rockchip/rk3399_grf.h
> new file mode 100644
> index 000000000000..0f94034e2e9a
> --- /dev/null
> +++ b/include/soc/rockchip/rk3399_grf.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Rockchip Generic Register Files definitions

Nit: s/Generic/General/

(that's what the TRMs say)

Robin.

> + *
> + * Copyright (c) 2018, Collabora Ltd.
> + * Author: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> + */
> +
> +#ifndef __SOC_RK3399_GRF_H
> +#define __SOC_RK3399_GRF_H
> +
> +/* PMU GRF Registers */
> +#define RK3399_PMUGRF_OS_REG2		0x308
> +#define RK3399_PMUGRF_DDRTYPE_SHIFT	13
> +#define RK3399_PMUGRF_DDRTYPE_MASK	7
> +#define RK3399_PMUGRF_DDRTYPE_DDR3	3
> +#define RK3399_PMUGRF_DDRTYPE_LPDDR2	5
> +#define RK3399_PMUGRF_DDRTYPE_LPDDR3	6
> +#define RK3399_PMUGRF_DDRTYPE_LPDDR4	7
> +
> +#endif
> 

Re: [RFC PATCH 07/10] clk: rockchip: set clk-ddr to GET_RATE_NOCACHE.

[Stephen Boyd]

Quoting Enric Balletbo i Serra (2018-05-14 14:16:07)
> From: Derek Basehore <dbasehore@chromium.org>
> 
> This adds the flag to the clk-ddr in rockchip to not use the cached
> rate for get_rate. This is to handle timeout error conditions in SMC
> for the set rate function.

We need some more information here. Why does timeout error condition in
set_rate() matter for get_rate()?

Re: [RFC PATCH 04/10] devfreq: rk3399_dmc / rockchip: pm_domains: Register notify to DMC driver.

[Chanwoo Choi]

Hi,

As I already commented[1], I think that it is not proper in order to pass
the devfreq instance to power_domain driver by separate defined function
(rockchip_pm_register_dmcfreq_notifier()).
[1] https://patchwork.kernel.org/patch/10349571/

Maybe, you could check the 'OF graph[1]' or 'device connection[2]'
for the device connection. Unfortunately, I'm not sure what is
best solution for this issue.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/graph.txt
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f2d9b66d84f3ff5ea3aff111e6a403e04fa8bf37


On 2018년 05월 15일 06:16, Enric Balletbo i Serra wrote:
> From: Lin Huang <hl@rock-chips.com>
> 
> The DMC (Dynamic Memory Interface) controller does a SiP call to the
> Trusted Firmware-A (TF-A) to change the DDR clock frequency. When this
> happens the TF-A writes to the PMU bus idle request register
> (PMU_BUS_IDLE_REQ) but at the same time it is possible that the Rockchip
> power domain driver writes to the same register. So, add a notification
> mechanism to ensure that the DMC and the PD driver does not access to this
> register at the same time.
> 
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> [rewrite commit message]
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> As I explained in the cover letter I have doubts regarding this patch
> but I did not find another way to do it. So I will appreciate any
> feedback on this.
> 
>  drivers/devfreq/rk3399_dmc.c      |  7 ++++++
>  drivers/soc/rockchip/pm_domains.c | 36 +++++++++++++++++++++++++++++++
>  include/soc/rockchip/rk3399_dmc.h | 14 ++++++++++++
>  3 files changed, 57 insertions(+)
>  create mode 100644 include/soc/rockchip/rk3399_dmc.h
> 
> diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
> index cc1bbca3fb15..2c4985a501cb 100644
> --- a/drivers/devfreq/rk3399_dmc.c
> +++ b/drivers/devfreq/rk3399_dmc.c
> @@ -28,6 +28,7 @@
>  #include <linux/rwsem.h>
>  #include <linux/suspend.h>
>  
> +#include <soc/rockchip/rk3399_dmc.h>
>  #include <soc/rockchip/rk3399_grf.h>
>  #include <soc/rockchip/rockchip_sip.h>
>  
> @@ -443,6 +444,12 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
>  	data->dev = dev;
>  	platform_set_drvdata(pdev, data);
>  
> +	rockchip_pm_register_dmcfreq_notifier(data->devfreq);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to register dmcfreq notifier\n");
> +		return ret;
> +	}
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/soc/rockchip/pm_domains.c b/drivers/soc/rockchip/pm_domains.c
> index 53efc386b1ad..b0e66f24b3e3 100644
> --- a/drivers/soc/rockchip/pm_domains.c
> +++ b/drivers/soc/rockchip/pm_domains.c
> @@ -8,6 +8,7 @@
>   * published by the Free Software Foundation.
>   */
>  
> +#include <linux/devfreq.h>
>  #include <linux/io.h>
>  #include <linux/iopoll.h>
>  #include <linux/err.h>
> @@ -76,9 +77,13 @@ struct rockchip_pmu {
>  	const struct rockchip_pmu_info *info;
>  	struct mutex mutex; /* mutex lock for pmu */
>  	struct genpd_onecell_data genpd_data;
> +	struct devfreq *devfreq;
> +	struct notifier_block dmc_nb;
>  	struct generic_pm_domain *domains[];
>  };
>  
> +static struct rockchip_pmu *dmc_pmu;
> +
>  #define to_rockchip_pd(gpd) container_of(gpd, struct rockchip_pm_domain, genpd)
>  
>  #define DOMAIN(pwr, status, req, idle, ack, wakeup)	\
> @@ -601,6 +606,35 @@ static int rockchip_pm_add_subdomain(struct rockchip_pmu *pmu,
>  	return error;
>  }
>  
> +static int rk3399_dmcfreq_notify(struct notifier_block *nb,
> +				 unsigned long event, void *data)
> +{
> +	if (event == DEVFREQ_PRECHANGE)
> +		mutex_lock(&dmc_pmu->mutex);
> +	else if (event == DEVFREQ_POSTCHANGE)
> +		mutex_unlock(&dmc_pmu->mutex);
> +
> +	return NOTIFY_OK;
> +}
> +
> +int rockchip_pm_register_dmcfreq_notifier(struct devfreq *devfreq)
> +{
> +	int ret;
> +
> +	if (!dmc_pmu)
> +		return -EPROBE_DEFER;
> +
> +	dmc_pmu->devfreq = devfreq;
> +	dmc_pmu->dmc_nb.notifier_call = rk3399_dmcfreq_notify;
> +	ret = devm_devfreq_register_notifier(devfreq->dev.parent,
> +					     dmc_pmu->devfreq,
> +					     &dmc_pmu->dmc_nb,
> +					     DEVFREQ_TRANSITION_NOTIFIER);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(rockchip_pm_register_dmcfreq_notifier);
> +
>  static int rockchip_pm_domain_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -694,6 +728,8 @@ static int rockchip_pm_domain_probe(struct platform_device *pdev)
>  		goto err_out;
>  	}
>  
> +	dmc_pmu = pmu;
> +
>  	return 0;
>  
>  err_out:
> diff --git a/include/soc/rockchip/rk3399_dmc.h b/include/soc/rockchip/rk3399_dmc.h
> new file mode 100644
> index 000000000000..031a62607f61
> --- /dev/null
> +++ b/include/soc/rockchip/rk3399_dmc.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2016-2018, Fuzhou Rockchip Electronics Co., Ltd
> + * Author: Lin Huang <hl@rock-chips.com>
> + */
> +
> +#ifndef __SOC_RK3399_DMC_H
> +#define __SOC_RK3399_DMC_H
> +
> +#include <linux/devfreq.h>
> +
> +int rockchip_pm_register_dmcfreq_notifier(struct devfreq *devfreq);
> +
> +#endif
> 

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [RFC PATCH 02/10] dt-bindings: devfreq: rk3399_dmc: Add rockchip,pmu phandle.

[Rob Herring]

On Mon, May 14, 2018 at 11:16:02PM +0200, Enric Balletbo i Serra wrote:
> The Rockchip DMC (Dynamic Memory Interface) needs to access to the PMU
> general register files to know the DRAM type, so add a phandle to the
> syscon that manages these registers.
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> 
>  Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt | 2 ++
>  1 file changed, 2 insertions(+)

Acked-by: Rob Herring <robh@kernel.org>

Re: [RFC PATCH 09/10] arm64: dts: rk3399: Add dfi and dmc nodes.

[Rob Herring]

On Mon, May 14, 2018 at 11:16:09PM +0200, Enric Balletbo i Serra wrote:
> From: Lin Huang <hl@rock-chips.com>
> 
> These are required to support DDR DVFS on rk3399 platform. The patch also
> introduces two new files (rk3399-dram.h and rk3399-dram-default-timing)
> with default DRAM settings.
> 
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> 
>  .../rockchip/rk3399-dram-default-timing.dtsi  | 38 ++++++++++
>  arch/arm64/boot/dts/rockchip/rk3399-dram.h    | 73 +++++++++++++++++++
>  .../boot/dts/rockchip/rk3399-op1-opp.dtsi     | 29 ++++++++
>  arch/arm64/boot/dts/rockchip/rk3399.dtsi      | 20 +++++
>  4 files changed, 160 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-dram-default-timing.dtsi
>  create mode 100644 arch/arm64/boot/dts/rockchip/rk3399-dram.h
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-dram-default-timing.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-dram-default-timing.dtsi
> new file mode 100644
> index 000000000000..4dfe3e1d8bdf
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-dram-default-timing.dtsi
> @@ -0,0 +1,38 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR X11)
> +/*
> + * Copyright (c) 2016-2018, Fuzhou Rockchip Electronics Co., Ltd
> + *
> + * Author: Lin Huang <hl@rock-chips.com>
> + */
> +
> +#include "rk3399-dram.h"
> +
> +rockchip,ddr3_speed_bin = <21>;
> +rockchip,pd_idle = <0x40>;
> +rockchip,sr_idle = <0x2>;

Don't do includes this way please. These should go under a node.

> +rockchip,sr_mc_gate_idle = <0x3>;
> +rockchip,srpd_lite_idle	= <0x4>;
> +rockchip,standby_idle = <0x2000>;
> +rockchip,dram_dll_dis_freq = <300000000>;
> +rockchip,phy_dll_dis_freq = <125000000>;
> +rockchip,auto_pd_dis_freq = <666000000>;
> +rockchip,ddr3_odt_dis_freq = <333000000>;
> +rockchip,ddr3_drv = <DDR3_DS_40ohm>;
> +rockchip,ddr3_odt = <DDR3_ODT_120ohm>;
> +rockchip,phy_ddr3_ca_drv = <PHY_DRV_ODT_40>;
> +rockchip,phy_ddr3_dq_drv = <PHY_DRV_ODT_40>;
> +rockchip,phy_ddr3_odt = <PHY_DRV_ODT_240>;
> +rockchip,lpddr3_odt_dis_freq = <333000000>;
> +rockchip,lpddr3_drv = <LP3_DS_34ohm>;
> +rockchip,lpddr3_odt = <LP3_ODT_240ohm>;
> +rockchip,phy_lpddr3_ca_drv = <PHY_DRV_ODT_40>;
> +rockchip,phy_lpddr3_dq_drv = <PHY_DRV_ODT_40>;
> +rockchip,phy_lpddr3_odt = <PHY_DRV_ODT_240>;
> +rockchip,lpddr4_odt_dis_freq = <333000000>;
> +rockchip,lpddr4_drv = <LP4_PDDS_60ohm>;
> +rockchip,lpddr4_dq_odt = <LP4_DQ_ODT_40ohm>;
> +rockchip,lpddr4_ca_odt = <LP4_CA_ODT_40ohm>;
> +rockchip,phy_lpddr4_ca_drv = <PHY_DRV_ODT_40>;
> +rockchip,phy_lpddr4_ck_cs_drv = <PHY_DRV_ODT_80>;
> +rockchip,phy_lpddr4_dq_drv = <PHY_DRV_ODT_80>;
> +rockchip,phy_lpddr4_odt = <PHY_DRV_ODT_60>;

Re: [RFC PATCH 03/10] devfreq: rk3399_dmc: Pass ODT and auto power down parameters to TF-A.

[Enric Balletbo Serra]

Hi Chanwoo,

I'll send a new version soon, just wanted to ask some questions here. See below.

Missatge de Chanwoo Choi <cw00.choi@samsung.com> del dia dt., 15 de
maig 2018 a les 0:21:
>
> Hi,
>
> On 2018년 05월 15일 06:16, Enric Balletbo i Serra wrote:
> > Trusted Firmware-A (TF-A) for rk3399 implements a SiP call to get the
> > on-die termination (ODT) and auto power down parameters from kernel,
> > this patch adds the functionality to do this. Also, if DDR clock
> > frequency is lower than the on-die termination (ODT) disable frequency
> > this driver should disable the DDR ODT.
>
> I have a question.
> 'disable frequency' is the same meaning of 'disable the DDR ODT'?
>

Yes, the DT defines an odt_dis_freq parameter, when the DDR frequency
is less than the value in this parameter we disable the ODT on the
DRAM.

> >
> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > ---
> >
> >  drivers/devfreq/rk3399_dmc.c        | 50 ++++++++++++++++++++++++++++-
> >  include/soc/rockchip/rockchip_sip.h |  1 +
> >  2 files changed, 50 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
> > index d5c03e5abe13..cc1bbca3fb15 100644
> > --- a/drivers/devfreq/rk3399_dmc.c
> > +++ b/drivers/devfreq/rk3399_dmc.c
> > @@ -18,14 +18,17 @@
> >  #include <linux/devfreq.h>
> >  #include <linux/devfreq-event.h>
> >  #include <linux/interrupt.h>
> > +#include <linux/mfd/syscon.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/pm_opp.h>
> > +#include <linux/regmap.h>
> >  #include <linux/regulator/consumer.h>
> >  #include <linux/rwsem.h>
> >  #include <linux/suspend.h>
> >
> > +#include <soc/rockchip/rk3399_grf.h>
> >  #include <soc/rockchip/rockchip_sip.h>
> >
> >  struct dram_timing {
> > @@ -69,8 +72,11 @@ struct rk3399_dmcfreq {
> >       struct mutex lock;
> >       struct dram_timing timing;
> >       struct regulator *vdd_center;
> > +     struct regmap *regmap_pmu;
> >       unsigned long rate, target_rate;
> >       unsigned long volt, target_volt;
> > +     unsigned int odt_dis_freq;
> > +     int odt_pd_arg0, odt_pd_arg1;
> >  };
> >
> >  static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
> > @@ -80,6 +86,8 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
> >       struct dev_pm_opp *opp;
> >       unsigned long old_clk_rate = dmcfreq->rate;
> >       unsigned long target_volt, target_rate;
> > +     struct arm_smccc_res res;
> > +     int dram_flag;
> >       int err;
> >
> >       opp = devfreq_recommended_opp(dev, freq, flags);
> > @@ -95,6 +103,15 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
> >
> >       mutex_lock(&dmcfreq->lock);
> >
> > +     dram_flag = 0;
>
> Also, if dram_flag is 0, it mean that disable ODT frequency?

Yes, not a good name, maybe I should just rename it to odt_enable to
be more clear.

> If it's right, you better to define the precise variables as following
> instead of just integer(0 or 1).
> For example,
> - ROCKCHIP_SIP_DRAM_FREQ_ENABLE
> - ROCKCHIP_SIP_DRAM_FREQ_DISABLE
>
> > +     if (target_rate >= dmcfreq->odt_dis_freq)
> > +             dram_flag = 1;
> > +
> > +     arm_smccc_smc(ROCKCHIP_SIP_DRAM_FREQ, dmcfreq->odt_pd_arg0,
> > +                   dmcfreq->odt_pd_arg1,
> > +                   ROCKCHIP_SIP_CONFIG_DRAM_SET_ODT_PD,
> > +                   dram_flag, 0, 0, 0, &res);
> > +
>
> This operation is special for only rk3399_dmc. It is difficult
> to understand what to do. I recommend you better to add the detailed comment
> with code.

Will do.

>
> >       /*
> >        * If frequency scaling from low to high, adjust voltage first.
> >        * If frequency scaling from high to low, adjust frequency first.
> > @@ -294,11 +311,13 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
> >  {
> >       struct arm_smccc_res res;
> >       struct device *dev = &pdev->dev;
> > -     struct device_node *np = pdev->dev.of_node;
> > +     struct device_node *np = pdev->dev.of_node, *node;
> >       struct rk3399_dmcfreq *data;
> >       int ret, index, size;
> >       uint32_t *timing;
> >       struct dev_pm_opp *opp;
> > +     u32 ddr_type;
> > +     u32 val;
> >
> >       data = devm_kzalloc(dev, sizeof(struct rk3399_dmcfreq), GFP_KERNEL);
> >       if (!data)
> > @@ -334,6 +353,29 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
> >               return ret;
> >       }
> >
> > +     /* 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);
> > +     }
> > +
> > +     /* Get DDR type */
> > +     regmap_read(data->regmap_pmu, RK3399_PMUGRF_OS_REG2, &val);
> > +     ddr_type = (val >> RK3399_PMUGRF_DDRTYPE_SHIFT) &
> > +                 RK3399_PMUGRF_DDRTYPE_MASK;
> > +
> > +     /* Get the odt_dis_freq parameter in function of the DDR type */
> > +     if (ddr_type == RK3399_PMUGRF_DDRTYPE_DDR3)
> > +             data->odt_dis_freq = data->timing.ddr3_odt_dis_freq;
> > +     else if (ddr_type == RK3399_PMUGRF_DDRTYPE_LPDDR3)
> > +             data->odt_dis_freq = data->timing.lpddr3_odt_dis_freq;
> > +     else if (ddr_type == RK3399_PMUGRF_DDRTYPE_LPDDR4)
> > +             data->odt_dis_freq = data->timing.lpddr4_odt_dis_freq;
> > +     else
> > +             return -EINVAL;
> > +
>
> how about using 'switch' statement?
>

Ok

> >       /*
> >        * Get dram timing and pass it to arm trust firmware,
> >        * the dram drvier in arm trust firmware will get these
> > @@ -358,6 +400,12 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
> >                     ROCKCHIP_SIP_CONFIG_DRAM_INIT,
> >                     0, 0, 0, 0, &res);
> >
> > +     data->odt_pd_arg0 = (data->timing.sr_idle & 0xff) |
> > +                         ((data->timing.sr_mc_gate_idle & 0xff) << 8) |
> > +                         ((data->timing.standby_idle & 0xffff) << 16);
> > +     data->odt_pd_arg1 = (data->timing.pd_idle & 0xfff) |
> > +                         ((data->timing.srpd_lite_idle & 0xfff) << 16);
> > +
>
> odt_pd_arg0 and odt_pd_arg1 might be used for disabling/enabling the ODT frequency.
> As I commented, it depend on only rk3399_dmc. You better to add detailed comment.
>

Ok

> And I prefer to define the XXX_SHIFT/XXX_MASK definition instead of
> using 8/16/0xff/0xffff for the readability.
>

I tried to add the XXX_SHIFT/XXX_MASK definitions and IMHO the
readability is worst if I use a maximum line length of 80 characters.
These masks are only used here, let me try to convince you by adding a
good doc in the next version and if you still prefer I add the
definition I'll do.

> >       /*
> >        * We add a devfreq driver to our parent since it has a device tree node
> >        * with operating points.
> > diff --git a/include/soc/rockchip/rockchip_sip.h b/include/soc/rockchip/rockchip_sip.h
> > index 7e28092c4d3d..ad9482c56797 100644
> > --- a/include/soc/rockchip/rockchip_sip.h
> > +++ b/include/soc/rockchip/rockchip_sip.h
> > @@ -23,5 +23,6 @@
> >  #define ROCKCHIP_SIP_CONFIG_DRAM_GET_RATE    0x05
> >  #define ROCKCHIP_SIP_CONFIG_DRAM_CLR_IRQ     0x06
> >  #define ROCKCHIP_SIP_CONFIG_DRAM_SET_PARAM   0x07
> > +#define ROCKCHIP_SIP_CONFIG_DRAM_SET_ODT_PD  0x08
> >
> >  #endif
> >
>
>
> --
> Best Regards,
> Chanwoo Choi
> Samsung Electronics
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
Cheers,
 Enric

Re: [RFC PATCH 03/10] devfreq: rk3399_dmc: Pass ODT and auto power down parameters to TF-A.

[Chanwoo Choi]

Hi Enric

2018-06-16 19:15 GMT+09:00 Enric Balletbo Serra <eballetbo@gmail.com>:
> Hi Chanwoo,
>
> I'll send a new version soon, just wanted to ask some questions here. See below.
>
> Missatge de Chanwoo Choi <cw00.choi@samsung.com> del dia dt., 15 de
> maig 2018 a les 0:21:
>>
>> Hi,
>>
>> On 2018년 05월 15일 06:16, Enric Balletbo i Serra wrote:
>> > Trusted Firmware-A (TF-A) for rk3399 implements a SiP call to get the
>> > on-die termination (ODT) and auto power down parameters from kernel,
>> > this patch adds the functionality to do this. Also, if DDR clock
>> > frequency is lower than the on-die termination (ODT) disable frequency
>> > this driver should disable the DDR ODT.
>>
>> I have a question.
>> 'disable frequency' is the same meaning of 'disable the DDR ODT'?
>>
>
> Yes, the DT defines an odt_dis_freq parameter, when the DDR frequency
> is less than the value in this parameter we disable the ODT on the
> DRAM.
>
>> >
>> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> > ---
>> >
>> >  drivers/devfreq/rk3399_dmc.c        | 50 ++++++++++++++++++++++++++++-
>> >  include/soc/rockchip/rockchip_sip.h |  1 +
>> >  2 files changed, 50 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
>> > index d5c03e5abe13..cc1bbca3fb15 100644
>> > --- a/drivers/devfreq/rk3399_dmc.c
>> > +++ b/drivers/devfreq/rk3399_dmc.c
>> > @@ -18,14 +18,17 @@
>> >  #include <linux/devfreq.h>
>> >  #include <linux/devfreq-event.h>
>> >  #include <linux/interrupt.h>
>> > +#include <linux/mfd/syscon.h>
>> >  #include <linux/module.h>
>> >  #include <linux/of.h>
>> >  #include <linux/platform_device.h>
>> >  #include <linux/pm_opp.h>
>> > +#include <linux/regmap.h>
>> >  #include <linux/regulator/consumer.h>
>> >  #include <linux/rwsem.h>
>> >  #include <linux/suspend.h>
>> >
>> > +#include <soc/rockchip/rk3399_grf.h>
>> >  #include <soc/rockchip/rockchip_sip.h>
>> >
>> >  struct dram_timing {
>> > @@ -69,8 +72,11 @@ struct rk3399_dmcfreq {
>> >       struct mutex lock;
>> >       struct dram_timing timing;
>> >       struct regulator *vdd_center;
>> > +     struct regmap *regmap_pmu;
>> >       unsigned long rate, target_rate;
>> >       unsigned long volt, target_volt;
>> > +     unsigned int odt_dis_freq;
>> > +     int odt_pd_arg0, odt_pd_arg1;
>> >  };
>> >
>> >  static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
>> > @@ -80,6 +86,8 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
>> >       struct dev_pm_opp *opp;
>> >       unsigned long old_clk_rate = dmcfreq->rate;
>> >       unsigned long target_volt, target_rate;
>> > +     struct arm_smccc_res res;
>> > +     int dram_flag;
>> >       int err;
>> >
>> >       opp = devfreq_recommended_opp(dev, freq, flags);
>> > @@ -95,6 +103,15 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
>> >
>> >       mutex_lock(&dmcfreq->lock);
>> >
>> > +     dram_flag = 0;
>>
>> Also, if dram_flag is 0, it mean that disable ODT frequency?
>
> Yes, not a good name, maybe I should just rename it to odt_enable to
> be more clear.
>
>> If it's right, you better to define the precise variables as following
>> instead of just integer(0 or 1).
>> For example,
>> - ROCKCHIP_SIP_DRAM_FREQ_ENABLE
>> - ROCKCHIP_SIP_DRAM_FREQ_DISABLE
>>
>> > +     if (target_rate >= dmcfreq->odt_dis_freq)
>> > +             dram_flag = 1;
>> > +
>> > +     arm_smccc_smc(ROCKCHIP_SIP_DRAM_FREQ, dmcfreq->odt_pd_arg0,
>> > +                   dmcfreq->odt_pd_arg1,
>> > +                   ROCKCHIP_SIP_CONFIG_DRAM_SET_ODT_PD,
>> > +                   dram_flag, 0, 0, 0, &res);
>> > +
>>
>> This operation is special for only rk3399_dmc. It is difficult
>> to understand what to do. I recommend you better to add the detailed comment
>> with code.
>
> Will do.
>
>>
>> >       /*
>> >        * If frequency scaling from low to high, adjust voltage first.
>> >        * If frequency scaling from high to low, adjust frequency first.
>> > @@ -294,11 +311,13 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
>> >  {
>> >       struct arm_smccc_res res;
>> >       struct device *dev = &pdev->dev;
>> > -     struct device_node *np = pdev->dev.of_node;
>> > +     struct device_node *np = pdev->dev.of_node, *node;
>> >       struct rk3399_dmcfreq *data;
>> >       int ret, index, size;
>> >       uint32_t *timing;
>> >       struct dev_pm_opp *opp;
>> > +     u32 ddr_type;
>> > +     u32 val;
>> >
>> >       data = devm_kzalloc(dev, sizeof(struct rk3399_dmcfreq), GFP_KERNEL);
>> >       if (!data)
>> > @@ -334,6 +353,29 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
>> >               return ret;
>> >       }
>> >
>> > +     /* 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);
>> > +     }
>> > +
>> > +     /* Get DDR type */
>> > +     regmap_read(data->regmap_pmu, RK3399_PMUGRF_OS_REG2, &val);
>> > +     ddr_type = (val >> RK3399_PMUGRF_DDRTYPE_SHIFT) &
>> > +                 RK3399_PMUGRF_DDRTYPE_MASK;
>> > +
>> > +     /* Get the odt_dis_freq parameter in function of the DDR type */
>> > +     if (ddr_type == RK3399_PMUGRF_DDRTYPE_DDR3)
>> > +             data->odt_dis_freq = data->timing.ddr3_odt_dis_freq;
>> > +     else if (ddr_type == RK3399_PMUGRF_DDRTYPE_LPDDR3)
>> > +             data->odt_dis_freq = data->timing.lpddr3_odt_dis_freq;
>> > +     else if (ddr_type == RK3399_PMUGRF_DDRTYPE_LPDDR4)
>> > +             data->odt_dis_freq = data->timing.lpddr4_odt_dis_freq;
>> > +     else
>> > +             return -EINVAL;
>> > +
>>
>> how about using 'switch' statement?
>>
>
> Ok
>
>> >       /*
>> >        * Get dram timing and pass it to arm trust firmware,
>> >        * the dram drvier in arm trust firmware will get these
>> > @@ -358,6 +400,12 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
>> >                     ROCKCHIP_SIP_CONFIG_DRAM_INIT,
>> >                     0, 0, 0, 0, &res);
>> >
>> > +     data->odt_pd_arg0 = (data->timing.sr_idle & 0xff) |
>> > +                         ((data->timing.sr_mc_gate_idle & 0xff) << 8) |
>> > +                         ((data->timing.standby_idle & 0xffff) << 16);
>> > +     data->odt_pd_arg1 = (data->timing.pd_idle & 0xfff) |
>> > +                         ((data->timing.srpd_lite_idle & 0xfff) << 16);
>> > +
>>
>> odt_pd_arg0 and odt_pd_arg1 might be used for disabling/enabling the ODT frequency.
>> As I commented, it depend on only rk3399_dmc. You better to add detailed comment.
>>
>
> Ok
>
>> And I prefer to define the XXX_SHIFT/XXX_MASK definition instead of
>> using 8/16/0xff/0xffff for the readability.
>>
>
> I tried to add the XXX_SHIFT/XXX_MASK definitions and IMHO the
> readability is worst if I use a maximum line length of 80 characters.
> These masks are only used here, let me try to convince you by adding a
> good doc in the next version and if you still prefer I add the
> definition I'll do.

If you add the some description and it would be only used on here,
I don't force to add some definition such as _SHIFT, _MASK as you suggested.

>
>> >       /*
>> >        * We add a devfreq driver to our parent since it has a device tree node
>> >        * with operating points.
>> > diff --git a/include/soc/rockchip/rockchip_sip.h b/include/soc/rockchip/rockchip_sip.h
>> > index 7e28092c4d3d..ad9482c56797 100644
>> > --- a/include/soc/rockchip/rockchip_sip.h
>> > +++ b/include/soc/rockchip/rockchip_sip.h
>> > @@ -23,5 +23,6 @@
>> >  #define ROCKCHIP_SIP_CONFIG_DRAM_GET_RATE    0x05
>> >  #define ROCKCHIP_SIP_CONFIG_DRAM_CLR_IRQ     0x06
>> >  #define ROCKCHIP_SIP_CONFIG_DRAM_SET_PARAM   0x07
>> > +#define ROCKCHIP_SIP_CONFIG_DRAM_SET_ODT_PD  0x08
>> >
>> >  #endif
>> >
>>
>>
>> --
>> Best Regards,
>> Chanwoo Choi
>> Samsung Electronics
>>
>> _______________________________________________
>> Linux-rockchip mailing list
>> Linux-rockchip@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
> Cheers,
>  Enric



-- 
Best Regards,
Chanwoo Choi
Samsung Electronics