[PATCH v2 0/5] Add support for drm/rockchip to dynamically control the DDR frequency.

[PATCH v2 0/5] Add support for drm/rockchip to dynamically control the DDR frequency.

[Gaël PORTAY]

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.

Waiting for your feedback.

Best regards,
Gaël

Changes in v2:
- [PATCH 1/8] Really add Acked-by: Chanwoo Choi <cw00.choi@samsung.com>.
- [PATCH 4/8] Removed from patchset.
- [PATCH 5/8] Removed from patchset.
- [PATCH 6/8] Removed from patchset.
- [PATCH 7/8] Reword the commit message to reflect the removal of
              rk3390-dram-default-timing.dts in v1.
- [PATCH 8/8] Move center-supply attribute of dmc node in file
              rk3399-gru-chromebook.dtsi (where ppvar_centerlogic is
	      defined).

Changes in v1:
- [RFC 1/10] Add Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
- [RFC 1/10] s/Generic/General/ (Robin Murphy)
- [RFC 2/10] Add reviewed and acked tags from Chanwoo Choi and Rob Herring
- [RFC 3/10] Add an explanation for platform SIP calls.
- [RFC 3/10] Change if statement for a switch.
- [RFC 3/10] Rename ddr_flag to odt_enable to be more clear.
- [RFC 4/10] Removed from the series. I did not found a use case where not holding the mutex causes the issue.
- [RFC 7/10] Removed from the series. I did not found a use case where this matters.
- [RFC 8/10] Move rk3399-dram.h to dt-includes.
- [RFC 8/10] Put sdram default values under the dmc node.
- [RFC 8/10] Removed rk3399-dram-default-timing.dts


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 (2):
  arm64: dts: rk3399: Add dfi and dmc nodes.
  arm64: dts: rockchip: Enable dmc and dfi nodes on gru.

 .../bindings/devfreq/rk3399_dmc.txt           |  2 +
 .../dts/rockchip/rk3399-gru-chromebook.dtsi   |  4 +
 arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi  | 20 +++++
 .../boot/dts/rockchip/rk3399-op1-opp.dtsi     | 29 ++++++++
 arch/arm64/boot/dts/rockchip/rk3399.dtsi      | 51 ++++++++++++-
 drivers/devfreq/event/rockchip-dfi.c          | 23 ++----
 drivers/devfreq/rk3399_dmc.c                  | 74 ++++++++++++++++++-
 include/dt-bindings/power/rk3399-dram.h       | 73 ++++++++++++++++++
 include/soc/rockchip/rk3399_grf.h             | 21 ++++++
 include/soc/rockchip/rockchip_sip.h           |  1 +
 10 files changed, 280 insertions(+), 18 deletions(-)
 create mode 100644 include/dt-bindings/power/rk3399-dram.h
 create mode 100644 include/soc/rockchip/rk3399_grf.h

-- 
2.21.0

[PATCH v2 1/5] devfreq: rockchip-dfi: Move GRF definitions to a common place.

[Gaël PORTAY]

From: Enric Balletbo i Serra <enric.balletbo@collabora.com>

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>
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
---

Changes in v2:
- [PATCH 1/8] Really add Acked-by: Chanwoo Choi <cw00.choi@samsung.com>.

Changes in v1:
- [RFC 1/10] Add Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
- [RFC 1/10] s/Generic/General/ (Robin Murphy)
- [RFC 4/10] Removed from the series. I did not found a use case where not holding the mutex causes the issue.
- [RFC 7/10] Removed from the series. I did not found a use case where this matters.

 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..3eebabcb2812
--- /dev/null
+++ b/include/soc/rockchip/rk3399_grf.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Rockchip General 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.21.0

[PATCH v2 2/5] dt-bindings: devfreq: rk3399_dmc: Add rockchip, pmu phandle.

[Gaël PORTAY]

From: Enric Balletbo i Serra <enric.balletbo@collabora.com>

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>
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
Acked-by: Rob Herring <robh@kernel.org>
---

Changes in v2: None

Changes in v1:
- [RFC 2/10] Add reviewed and acked tags from Chanwoo Choi and Rob Herring

 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.21.0

[PATCH v2 3/5] devfreq: rk3399_dmc: Pass ODT and auto power down parameters to TF-A.

[Gaël PORTAY]

From: Enric Balletbo i Serra <enric.balletbo@collabora.com>

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>
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
---

Changes in v2: None

Changes in v1:
- [RFC 3/10] Add an explanation for platform SIP calls.
- [RFC 3/10] Change if statement for a switch.
- [RFC 3/10] Rename ddr_flag to odt_enable to be more clear.

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

diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
index e795ad2b3f6b..c619dc4ac620 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;
+	bool odt_enable = false;
 	int err;
 
 	opp = devfreq_recommended_opp(dev, freq, flags);
@@ -95,6 +103,19 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
 
 	mutex_lock(&dmcfreq->lock);
 
+	if (target_rate >= dmcfreq->odt_dis_freq)
+		odt_enable = true;
+
+	/*
+	 * This makes a SMC call to the TF-A to set the DDR PD (power-down)
+	 * timings and to enable or disable the ODT (on-die termination)
+	 * resistors.
+	 */
+	arm_smccc_smc(ROCKCHIP_SIP_DRAM_FREQ, dmcfreq->odt_pd_arg0,
+		      dmcfreq->odt_pd_arg1,
+		      ROCKCHIP_SIP_CONFIG_DRAM_SET_ODT_PD,
+		      odt_enable, 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 +315,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 +357,34 @@ 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 */
+	switch (ddr_type) {
+	case RK3399_PMUGRF_DDRTYPE_DDR3:
+		data->odt_dis_freq = data->timing.ddr3_odt_dis_freq;
+		break;
+	case RK3399_PMUGRF_DDRTYPE_LPDDR3:
+		data->odt_dis_freq = data->timing.lpddr3_odt_dis_freq;
+		break;
+	case RK3399_PMUGRF_DDRTYPE_LPDDR4:
+		data->odt_dis_freq = data->timing.lpddr4_odt_dis_freq;
+		break;
+	default:
+		return -EINVAL;
+	};
+
 	/*
 	 * Get dram timing and pass it to arm trust firmware,
 	 * the dram drvier in arm trust firmware will get these
@@ -358,6 +409,27 @@ static int rk3399_dmcfreq_probe(struct platform_device *pdev)
 		      ROCKCHIP_SIP_CONFIG_DRAM_INIT,
 		      0, 0, 0, 0, &res);
 
+	/*
+	 * In TF-A there is a platform SIP call to set the PD (power-down)
+	 * timings and to enable or disable the ODT (on-die termination).
+	 * This call needs three arguments as follows:
+	 *
+	 * arg0:
+	 *     bit[0-7]   : sr_idle
+	 *     bit[8-15]  : sr_mc_gate_idle
+	 *     bit[16-31] : standby idle
+	 * arg1:
+	 *     bit[0-11]  : pd_idle
+	 *     bit[16-27] : srpd_lite_idle
+	 * arg2:
+	 *     bit[0]     : odt enable
+	 */
+	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.21.0

[PATCH v2 4/5] arm64: dts: rk3399: Add dfi and dmc nodes.

[Gaël PORTAY]

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

These are required to support DDR DVFS on rk3399 platform. The patch also
introduces a new file with default DRAM settings.

Signed-off-by: Lin Huang <hl@rock-chips.com>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Signed-off-by: Gaël PORTAY <gael.portay@collabora.com>
---

Changes in v2:
- [PATCH 7/8] Reword the commit message to reflect the removal of
              rk3390-dram-default-timing.dts in v1.

Changes in v1:
- [RFC 8/10] Move rk3399-dram.h to dt-includes.
- [RFC 8/10] Put sdram default values under the dmc node.
- [RFC 8/10] Removed rk3399-dram-default-timing.dts

 .../boot/dts/rockchip/rk3399-op1-opp.dtsi     | 29 ++++++++
 arch/arm64/boot/dts/rockchip/rk3399.dtsi      | 49 +++++++++++++
 include/dt-bindings/power/rk3399-dram.h       | 73 +++++++++++++++++++
 3 files changed, 151 insertions(+)
 create mode 100644 include/dt-bindings/power/rk3399-dram.h

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-op1-opp.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-op1-opp.dtsi
index 69cc9b05baa5..c9e7032b01a8 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-op1-opp.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-op1-opp.dtsi
@@ -110,6 +110,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 {
@@ -139,3 +164,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 db9d948c0b03..8fe86a3e7658 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
@@ -8,6 +8,7 @@
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/interrupt-controller/irq.h>
 #include <dt-bindings/pinctrl/rockchip.h>
+#include <dt-bindings/power/rk3399-dram.h>
 #include <dt-bindings/power/rk3399-power.h>
 #include <dt-bindings/thermal/thermal.h>
 
@@ -1885,6 +1886,54 @@
 		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";
+		status = "disabled";
+		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>;
+	};
+
 	pinctrl: pinctrl {
 		compatible = "rockchip,rk3399-pinctrl";
 		rockchip,grf = <&grf>;
diff --git a/include/dt-bindings/power/rk3399-dram.h b/include/dt-bindings/power/rk3399-dram.h
new file mode 100644
index 000000000000..4b3d4a79923b
--- /dev/null
+++ b/include/dt-bindings/power/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 */
-- 
2.21.0

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

[Gaël PORTAY]

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>
Signed-off-by: Gaël PORTAY <gael.portay@collabora.com>
---

Changes in v2:
- [PATCH 8/8] Move center-supply attribute of dmc node in file
              rk3399-gru-chromebook.dtsi (where ppvar_centerlogic is
	      defined).

Changes in v1: None

 .../dts/rockchip/rk3399-gru-chromebook.dtsi   |  4 ++++
 arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi  | 20 +++++++++++++++++++
 arch/arm64/boot/dts/rockchip/rk3399.dtsi      |  2 +-
 3 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru-chromebook.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-gru-chromebook.dtsi
index 931640e9aed4..cfb81356c61e 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-gru-chromebook.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-gru-chromebook.dtsi
@@ -400,3 +400,7 @@ ap_i2c_tp: &i2c5 {
 		rockchip,pins = <0 8 RK_FUNC_GPIO &pcfg_pull_none>;
 	};
 };
+
+&dmc {
+	center-supply = <&ppvar_centerlogic>;
+};
diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
index da03fa9c5662..d14dce679e7a 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
@@ -289,6 +289,12 @@
 	status = "okay";
 };
 
+&dmc_opp_table {
+	opp04 {
+		opp-suspend;
+	};
+};
+
 /*
  * Set some suspend operating points to avoid OVP in suspend
  *
@@ -368,6 +374,10 @@
 		<200000000>;
 };
 
+&display_subsystem {
+	devfreq = <&dmc>;
+};
+
 &emmc_phy {
 	status = "okay";
 };
@@ -489,6 +499,16 @@ ap_i2c_audio: &i2c8 {
 	status = "okay";
 };
 
+&dfi {
+	status = "okay";
+};
+
+&dmc {
+	status = "okay";
+	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 8fe86a3e7658..010b3e5267a0 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
@@ -156,7 +156,7 @@
 		};
 	};
 
-	display-subsystem {
+	display_subsystem: display-subsystem {
 		compatible = "rockchip,display-subsystem";
 		ports = <&vopl_out>, <&vopb_out>;
 	};
-- 
2.21.0

Re: [PATCH v2 3/5] devfreq: rk3399_dmc: Pass ODT and auto power down parameters to TF-A.

[Matthias Kaehlcke]

Hi Gaël,

On Tue, Mar 19, 2019 at 02:13:21PM -0400, Gaël PORTAY wrote:
> From: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> 
> 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>
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
> 
> Changes in v2: None
> 
> Changes in v1:
> - [RFC 3/10] Add an explanation for platform SIP calls.
> - [RFC 3/10] Change if statement for a switch.
> - [RFC 3/10] Rename ddr_flag to odt_enable to be more clear.
> 
>  drivers/devfreq/rk3399_dmc.c        | 74 ++++++++++++++++++++++++++++-
>  include/soc/rockchip/rockchip_sip.h |  1 +
>  2 files changed, 74 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
> ...
>  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;
> +	bool odt_enable = false;
>  	int err;
>  
>  	opp = devfreq_recommended_opp(dev, freq, flags);
> @@ -95,6 +103,19 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
>  
>  	mutex_lock(&dmcfreq->lock);
>  
> +	if (target_rate >= dmcfreq->odt_dis_freq)
> +		odt_enable = true;
> +
> +	/*
> +	 * This makes a SMC call to the TF-A to set the DDR PD (power-down)
> +	 * timings and to enable or disable the ODT (on-die termination)
> +	 * resistors.
> +	 */
> +	arm_smccc_smc(ROCKCHIP_SIP_DRAM_FREQ, dmcfreq->odt_pd_arg0,
> +		      dmcfreq->odt_pd_arg1,
> +		      ROCKCHIP_SIP_CONFIG_DRAM_SET_ODT_PD,
> +		      odt_enable, 0, 0, 0, &res);

Is it necessary/desirable to make this call for every frequency
change? IIUC it should be only needed when odt_enable changes and the
driver could track the state. If the DDR frequency doesn't change too
often and the overhead of the call is small it shouldn't be really
important though.

> @@ -294,11 +315,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 +357,34 @@ 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) {

The 'optional' part doesn't seem to be accurate: according to the
binding (https://lore.kernel.org/patchwork/patch/1052322/) the
property is required and the code below assumes that data->regmap_pmu
is set.

> +		data->regmap_pmu = syscon_node_to_regmap(node);
> +		if (IS_ERR(data->regmap_pmu))
> +			return PTR_ERR(data->regmap_pmu);
> +	}
> +
> +	/* Get DDR type */

nit: the comment doesn't add much value, thanks to good variable
naming this is evident from the code.

> +	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 */

nit: ditto

(if you prefer to keep these comments I'm not opposed to keeping them,
but I don't think they are needed)

Cheers

Matthias

Re: [PATCH v2 5/5] arm64: dts: rockchip: Enable dmc and dfi nodes on gru.

[Chanwoo Choi]

Hi Gaël,

On 19. 3. 20. 오전 3:13, Gaël PORTAY wrote:
> 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>
> Signed-off-by: Gaël PORTAY <gael.portay@collabora.com>
> ---
> 
> Changes in v2:
> - [PATCH 8/8] Move center-supply attribute of dmc node in file
>               rk3399-gru-chromebook.dtsi (where ppvar_centerlogic is
> 	      defined).
> 
> Changes in v1: None
> 
>  .../dts/rockchip/rk3399-gru-chromebook.dtsi   |  4 ++++
>  arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi  | 20 +++++++++++++++++++
>  arch/arm64/boot/dts/rockchip/rk3399.dtsi      |  2 +-
>  3 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru-chromebook.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-gru-chromebook.dtsi
> index 931640e9aed4..cfb81356c61e 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399-gru-chromebook.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-gru-chromebook.dtsi
> @@ -400,3 +400,7 @@ ap_i2c_tp: &i2c5 {
>  		rockchip,pins = <0 8 RK_FUNC_GPIO &pcfg_pull_none>;
>  	};
>  };
> +
> +&dmc {
> +	center-supply = <&ppvar_centerlogic>;
> +};
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
> index da03fa9c5662..d14dce679e7a 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
> @@ -289,6 +289,12 @@
>  	status = "okay";
>  };
>  
> +&dmc_opp_table {
> +	opp04 {
> +		opp-suspend;
> +	};
> +};
> +
>  /*
>   * Set some suspend operating points to avoid OVP in suspend
>   *
> @@ -368,6 +374,10 @@
>  		<200000000>;
>  };
>  
> +&display_subsystem {
> +	devfreq = <&dmc>;
> +};

When I checked the rockchip_drm_drv.c on linux-next.git (20190320),
there are no any codes about this. Maybe it should be removed.

> +
>  &emmc_phy {
>  	status = "okay";
>  };
> @@ -489,6 +499,16 @@ ap_i2c_audio: &i2c8 {
>  	status = "okay";
>  };
>  
> +&dfi {
> +	status = "okay";
> +};
> +
> +&dmc {
> +	status = "okay";
> +	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 8fe86a3e7658..010b3e5267a0 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> @@ -156,7 +156,7 @@
>  		};
>  	};
>  
> -	display-subsystem {
> +	display_subsystem: display-subsystem {
>  		compatible = "rockchip,display-subsystem";
>  		ports = <&vopl_out>, <&vopb_out>;
>  	};
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 5/5] arm64: dts: rockchip: Enable dmc and dfi nodes on gru.

[Gaël PORTAY]

Hi Chanwoo,

On Wed, Mar 20, 2019 at 02:05:21PM +0900, Chanwoo Choi wrote:
> ...
> > +&display_subsystem {
> > +	devfreq = <&dmc>;
> > +};
> 
> When I checked the rockchip_drm_drv.c on linux-next.git (20190320),
> there are no any codes about this. Maybe it should be removed.
>

Good catch! This is a artifact that remain after I removed two patches
from v1.

> > ...
> 
> -- 
> Best Regards,
> Chanwoo Choi
> Samsung Electronics
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Best Regards,
Gaël

Re: [PATCH v2 3/5] devfreq: rk3399_dmc: Pass ODT and auto power down parameters to TF-A.

[Gaël PORTAY]

Hi Matthias,

On Tue, Mar 19, 2019 at 05:33:52PM -0700, Matthias Kaehlcke wrote:
> ...
> > @@ -95,6 +103,19 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
> >  
> >  	mutex_lock(&dmcfreq->lock);
> >  
> > +	if (target_rate >= dmcfreq->odt_dis_freq)
> > +		odt_enable = true;
> > +
> > +	/*
> > +	 * This makes a SMC call to the TF-A to set the DDR PD (power-down)
> > +	 * timings and to enable or disable the ODT (on-die termination)
> > +	 * resistors.
> > +	 */
> > +	arm_smccc_smc(ROCKCHIP_SIP_DRAM_FREQ, dmcfreq->odt_pd_arg0,
> > +		      dmcfreq->odt_pd_arg1,
> > +		      ROCKCHIP_SIP_CONFIG_DRAM_SET_ODT_PD,
> > +		      odt_enable, 0, 0, 0, &res);
> 
> Is it necessary/desirable to make this call for every frequency
> change? IIUC it should be only needed when odt_enable changes and the
> driver could track the state. If the DDR frequency doesn't change too
> often and the overhead of the call is small it shouldn't be really
> important though.
>

I will test your solution first to make sure there is no regression to
run that call for frequency change only.

Also, the call takes around 300us.

> > ...
> > @@ -334,6 +357,34 @@ 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) {
> 
> The 'optional' part doesn't seem to be accurate: according to the
> binding (https://lore.kernel.org/patchwork/patch/1052322/) the
> property is required and the code below assumes that data->regmap_pmu
> is set.
>

Indeed.

> > +		data->regmap_pmu = syscon_node_to_regmap(node);
> > +		if (IS_ERR(data->regmap_pmu))
> > +			return PTR_ERR(data->regmap_pmu);
> > +	}
> > +
> > +	/* Get DDR type */
> 
> nit: the comment doesn't add much value, thanks to good variable
> naming this is evident from the code.
> 
> > +	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 */
> 
> nit: ditto
> 
> (if you prefer to keep these comments I'm not opposed to keeping them,
> but I don't think they are needed)
>

I will remove all the comments in the v3.

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

Regards,
Gaël

Re: [PATCH v2 3/5] devfreq: rk3399_dmc: Pass ODT and auto power down parameters to TF-A.

[Matthias Kaehlcke]

Hi Gaël,

On Wed, Mar 20, 2019 at 05:50:13PM -0400, Gaël PORTAY wrote:
> Hi Matthias,
> 
> On Tue, Mar 19, 2019 at 05:33:52PM -0700, Matthias Kaehlcke wrote:
> > ...
> > > @@ -95,6 +103,19 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
> > >  
> > >  	mutex_lock(&dmcfreq->lock);
> > >  
> > > +	if (target_rate >= dmcfreq->odt_dis_freq)
> > > +		odt_enable = true;
> > > +
> > > +	/*
> > > +	 * This makes a SMC call to the TF-A to set the DDR PD (power-down)
> > > +	 * timings and to enable or disable the ODT (on-die termination)
> > > +	 * resistors.
> > > +	 */
> > > +	arm_smccc_smc(ROCKCHIP_SIP_DRAM_FREQ, dmcfreq->odt_pd_arg0,
> > > +		      dmcfreq->odt_pd_arg1,
> > > +		      ROCKCHIP_SIP_CONFIG_DRAM_SET_ODT_PD,
> > > +		      odt_enable, 0, 0, 0, &res);
> > 
> > Is it necessary/desirable to make this call for every frequency
> > change? IIUC it should be only needed when odt_enable changes and the
> > driver could track the state. If the DDR frequency doesn't change too
> > often and the overhead of the call is small it shouldn't be really
> > important though.
> >
> 
> I will test your solution first to make sure there is no regression to
> run that call for frequency change only.

If there is no frequency change the function returns at the
beginning. My suggestion was to only do the call when 'odt_enable'
changes, i.e. when a change (up or down) passes the 'odt_dis_freq'
threshold.

> Also, the call takes around 300us.

Thanks for the info!

Matthias

Re: [PATCH v2 3/5] devfreq: rk3399_dmc: Pass ODT and auto power down parameters to TF-A.

[Gaël PORTAY]

Matthias,

On Wed, Mar 20, 2019 at 03:02:23PM -0700, Matthias Kaehlcke wrote:
> Hi Gaël,
> 
> On Wed, Mar 20, 2019 at 05:50:13PM -0400, Gaël PORTAY wrote:
> > Hi Matthias,
> > 
> > On Tue, Mar 19, 2019 at 05:33:52PM -0700, Matthias Kaehlcke wrote:
> > > ...
> > > > @@ -95,6 +103,19 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
> > > >  
> > > >  	mutex_lock(&dmcfreq->lock);
> > > >  
> > > > +	if (target_rate >= dmcfreq->odt_dis_freq)
> > > > +		odt_enable = true;
> > > > +
> > > > +	/*
> > > > +	 * This makes a SMC call to the TF-A to set the DDR PD (power-down)
> > > > +	 * timings and to enable or disable the ODT (on-die termination)
> > > > +	 * resistors.
> > > > +	 */
> > > > +	arm_smccc_smc(ROCKCHIP_SIP_DRAM_FREQ, dmcfreq->odt_pd_arg0,
> > > > +		      dmcfreq->odt_pd_arg1,
> > > > +		      ROCKCHIP_SIP_CONFIG_DRAM_SET_ODT_PD,
> > > > +		      odt_enable, 0, 0, 0, &res);
> > > 
> > > Is it necessary/desirable to make this call for every frequency
> > > change? IIUC it should be only needed when odt_enable changes and the
> > > driver could track the state. If the DDR frequency doesn't change too
> > > often and the overhead of the call is small it shouldn't be really
> > > important though.
> > >
> > 
> > I will test your solution first to make sure there is no regression to
> > run that call for frequency change only.
> 

Oops, I wanted to say when odt_enable changes since last call.

> If there is no frequency change the function returns at the
> beginning. My suggestion was to only do the call when 'odt_enable'
> changes, i.e. when a change (up or down) passes the 'odt_dis_freq'
> threshold.
> 

So, for a reason that I ignore, if we try to save unecessary calls to
ROCKCHIP_SIP_CONFIG_DRAM_SET_ODT_PD (odt_enable has not changed since
last call), we get stalled in the call to ROCKCHIP_SIP_CONFIG_SET_RAGE
that follows. The function arm_smccc_smc never returns and the device
hard hang.

Thanks to your remark, I have also fixed an issue with the odt_dis_freq
value. Its value is initialized to 0 in the probe function. Thus the
odt_enable is always true (target_rate > 0). I moved its initialization
after the timings are parsed from the device-tree; its value is now none
zero (333000000 in my case).

> > Also, the call takes around 300us.
> 
> Thanks for the info!
> 
> Matthias
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Gaël

Re: [PATCH v2 3/5] devfreq: rk3399_dmc: Pass ODT and auto power down parameters to TF-A.

[Matthias Kaehlcke]

Hi Gaël,

On Thu, Mar 21, 2019 at 07:10:55PM -0400, Gaël PORTAY wrote:
> Matthias,
> 
> On Wed, Mar 20, 2019 at 03:02:23PM -0700, Matthias Kaehlcke wrote:
> > Hi Gaël,
> > 
> > On Wed, Mar 20, 2019 at 05:50:13PM -0400, Gaël PORTAY wrote:
> > > Hi Matthias,
> > > 
> > > On Tue, Mar 19, 2019 at 05:33:52PM -0700, Matthias Kaehlcke wrote:
> > > > ...
> > > > > @@ -95,6 +103,19 @@ static int rk3399_dmcfreq_target(struct device *dev, unsigned long *freq,
> > > > >  
> > > > >  	mutex_lock(&dmcfreq->lock);
> > > > >  
> > > > > +	if (target_rate >= dmcfreq->odt_dis_freq)
> > > > > +		odt_enable = true;
> > > > > +
> > > > > +	/*
> > > > > +	 * This makes a SMC call to the TF-A to set the DDR PD (power-down)
> > > > > +	 * timings and to enable or disable the ODT (on-die termination)
> > > > > +	 * resistors.
> > > > > +	 */
> > > > > +	arm_smccc_smc(ROCKCHIP_SIP_DRAM_FREQ, dmcfreq->odt_pd_arg0,
> > > > > +		      dmcfreq->odt_pd_arg1,
> > > > > +		      ROCKCHIP_SIP_CONFIG_DRAM_SET_ODT_PD,
> > > > > +		      odt_enable, 0, 0, 0, &res);
> > > > 
> > > > Is it necessary/desirable to make this call for every frequency
> > > > change? IIUC it should be only needed when odt_enable changes and the
> > > > driver could track the state. If the DDR frequency doesn't change too
> > > > often and the overhead of the call is small it shouldn't be really
> > > > important though.
> > > >
> > > 
> > > I will test your solution first to make sure there is no regression to
> > > run that call for frequency change only.
> > 
> 
> Oops, I wanted to say when odt_enable changes since last call.
> 
> > If there is no frequency change the function returns at the
> > beginning. My suggestion was to only do the call when 'odt_enable'
> > changes, i.e. when a change (up or down) passes the 'odt_dis_freq'
> > threshold.
> > 
> 
> So, for a reason that I ignore, if we try to save unecessary calls to
> ROCKCHIP_SIP_CONFIG_DRAM_SET_ODT_PD (odt_enable has not changed since
> last call), we get stalled in the call to ROCKCHIP_SIP_CONFIG_SET_RAGE
> that follows. The function arm_smccc_smc never returns and the device
> hard hang.

Thanks for giving it a try!

Did your code ensure to perform the SMC call for the first frequency
change? If not the problem could be that the DDR PD timings and ODT
resistors are not properly configured for the new frequency.

In case you already did this or it doesn't help I think it's fine to
just do the call always, we can always revisit this later.

> Thanks to your remark, I have also fixed an issue with the odt_dis_freq
> value. Its value is initialized to 0 in the probe function. Thus the
> odt_enable is always true (target_rate > 0). I moved its initialization
> after the timings are parsed from the device-tree; its value is now none
> zero (333000000 in my case).

Great!

Re: [PATCH v2 3/5] devfreq: rk3399_dmc: Pass ODT and auto power down parameters to TF-A.

[Gaël PORTAY]

Hi Matthias,

On Thu, Mar 21, 2019 at 05:01:07PM -0700, Matthias Kaehlcke wrote:
> > ...
> > 
> > So, for a reason that I ignore, if we try to save unecessary calls to
> > ROCKCHIP_SIP_CONFIG_DRAM_SET_ODT_PD (odt_enable has not changed since
> > last call), we get stalled in the call to ROCKCHIP_SIP_CONFIG_SET_RAGE
> > that follows. The function arm_smccc_smc never returns and the device
> > hard hang.
> 
> Thanks for giving it a try!
> 
> Did your code ensure to perform the SMC call for the first frequency
> change? If not the problem could be that the DDR PD timings and ODT
> resistors are not properly configured for the new frequency.
> 

The DRAM_ODT_PD SMC call is supposed to be performed before the
DRAM_SET_RATE; unless someone else is doing the set_rate.

Does the ODT resistors should be configured for every existing
frequency?

> In case you already did this or it doesn't help I think it's fine to
> just do the call always, we can always revisit this later.
> 

Okay, sounds good.

> > Thanks to your remark, I have also fixed an issue with the odt_dis_freq
> > value. Its value is initialized to 0 in the probe function. Thus the
> > odt_enable is always true (target_rate > 0). I moved its initialization
> > after the timings are parsed from the device-tree; its value is now none
> > zero (333000000 in my case).
> 
> Great!

Gael

Re: [PATCH v2 3/5] devfreq: rk3399_dmc: Pass ODT and auto power down parameters to TF-A.

[Matthias Kaehlcke]

On Fri, Mar 22, 2019 at 08:45:26AM -0400, Gaël PORTAY wrote:
> Hi Matthias,
> 
> On Thu, Mar 21, 2019 at 05:01:07PM -0700, Matthias Kaehlcke wrote:
> > > ...
> > > 
> > > So, for a reason that I ignore, if we try to save unecessary calls to
> > > ROCKCHIP_SIP_CONFIG_DRAM_SET_ODT_PD (odt_enable has not changed since
> > > last call), we get stalled in the call to ROCKCHIP_SIP_CONFIG_SET_RAGE
> > > that follows. The function arm_smccc_smc never returns and the device
> > > hard hang.
> > 
> > Thanks for giving it a try!
> > 
> > Did your code ensure to perform the SMC call for the first frequency
> > change? If not the problem could be that the DDR PD timings and ODT
> > resistors are not properly configured for the new frequency.
> > 
> 
> The DRAM_ODT_PD SMC call is supposed to be performed before the
> DRAM_SET_RATE; unless someone else is doing the set_rate.

However earlier the call wasn't done at all, and that didn't cause
problems.

> Does the ODT resistors should be configured for every existing
> frequency?

I don't have any background here. My initial assumption would be that
it should be enough to re-configure them when the frequency passes the
threshold in either direction.

Anyway, IIUC there shouldn't be more than 5 frequency changes per
second (polling_ms = 200), and likely no all of them would pass the
threshold, so it seems limiting the calls (if possible) would be a
micro-optimization and is probably not worth the hassle :)

Thanks

Matthias