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

Commit Message

Gaël PORTAY March 19, 2019, 6:13 p.m. UTC

From: Enric Balletbo i Serra <[e-mail]>

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 <[e-mail]>
Reviewed-by: Chanwoo Choi <[e-mail]>
---

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(-)

Comments

Matthias Kaehlcke March 20, 2019, 12:33 a.m. UTC | #1

Hi Gaël,

On Tue, Mar 19, 2019 at 02:13:21PM -0400, Gaël PORTAY wrote:
> From: Enric Balletbo i Serra <[e-mail]>
> 
> 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 <[e-mail]>
> Reviewed-by: Chanwoo Choi <[e-mail]>
> ---
> 
> 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

Gaël PORTAY March 20, 2019, 9:50 p.m. UTC | #2

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
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Regards,
Gaël

Matthias Kaehlcke March 20, 2019, 10:02 p.m. UTC | #3

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

Gaël PORTAY March 21, 2019, 11:10 p.m. UTC | #4

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
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Gaël

Matthias Kaehlcke March 22, 2019, 12:01 a.m. UTC | #5

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!

Gaël PORTAY March 22, 2019, 12:45 p.m. UTC | #6

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

Matthias Kaehlcke March 27, 2019, 12:24 a.m. UTC | #7

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

Patch

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