diff mbox series

[RFC,05/19] devfreq: imx8m-ddrc: Use the opps acquired from EL3

Message ID 1631554694-9599-6-git-send-email-abel.vesa@nxp.com
State New
Headers show
Series Add interconnect and devfreq support for i.MX8MQ | expand

Commit Message

Abel Vesa Sept. 13, 2021, 5:38 p.m. UTC
i.MX8M platforms get their dram OPPs from the EL3.
We don't need to duplicate that in the kernel dram dts node.
We should just trust the OPPs provided by the EL3.

Signed-off-by: Abel Vesa <abel.vesa@nxp.com>
---
 drivers/devfreq/imx8m-ddrc.c | 50 +++---------------------------------
 1 file changed, 3 insertions(+), 47 deletions(-)

Comments

Chanwoo Choi Sept. 15, 2021, 3:29 a.m. UTC | #1
Hi,

OPP is mandatory for devfreq driver. Also, must need to add
the OPP levels to  devicetree file, it is better to show
the supported OPP list for the developer who don't know
the detailed background of driver. If there are no any
critical issue. I prefer the existing approach for the readability.

On 21. 9. 14. 오전 2:38, Abel Vesa wrote:
> i.MX8M platforms get their dram OPPs from the EL3.
> We don't need to duplicate that in the kernel dram dts node.
> We should just trust the OPPs provided by the EL3.
> 
> Signed-off-by: Abel Vesa <abel.vesa@nxp.com>
> ---
>   drivers/devfreq/imx8m-ddrc.c | 50 +++---------------------------------
>   1 file changed, 3 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/devfreq/imx8m-ddrc.c b/drivers/devfreq/imx8m-ddrc.c
> index 583123bf2100..f18a5c3c1c03 100644
> --- a/drivers/devfreq/imx8m-ddrc.c
> +++ b/drivers/devfreq/imx8m-ddrc.c
> @@ -321,38 +321,9 @@ static int imx8m_ddrc_init_freq_info(struct device *dev)
>   		if (freq->dram_core_parent_index == 2 &&
>   				freq->dram_alt_parent_index == 0)
>   			return -ENODEV;
> -	}
> -
> -	return 0;
> -}
> -
> -static int imx8m_ddrc_check_opps(struct device *dev)
> -{
> -	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
> -	struct imx8m_ddrc_freq *freq_info;
> -	struct dev_pm_opp *opp;
> -	unsigned long freq;
> -	int i, opp_count;
> -
> -	/* Enumerate DT OPPs and disable those not supported by firmware */
> -	opp_count = dev_pm_opp_get_opp_count(dev);
> -	if (opp_count < 0)
> -		return opp_count;
> -	for (i = 0, freq = 0; i < opp_count; ++i, ++freq) {
> -		opp = dev_pm_opp_find_freq_ceil(dev, &freq);
> -		if (IS_ERR(opp)) {
> -			dev_err(dev, "Failed enumerating OPPs: %ld\n",
> -				PTR_ERR(opp));
> -			return PTR_ERR(opp);
> -		}
> -		dev_pm_opp_put(opp);
>   
> -		freq_info = imx8m_ddrc_find_freq(priv, freq);
> -		if (!freq_info) {
> -			dev_info(dev, "Disable unsupported OPP %luHz %luMT/s\n",
> -					freq, DIV_ROUND_CLOSEST(freq, 250000));
> -			dev_pm_opp_disable(dev, freq);
> -		}
> +		if (dev_pm_opp_add(dev, freq->rate * 250000, 0))
> +			return -ENODEV;
>   	}
>   
>   	return 0;
> @@ -360,7 +331,6 @@ static int imx8m_ddrc_check_opps(struct device *dev)
>   
>   static void imx8m_ddrc_exit(struct device *dev)
>   {
> -	dev_pm_opp_of_remove_table(dev);
>   }
>   
>   static int imx8m_ddrc_probe(struct platform_device *pdev)
> @@ -407,16 +377,7 @@ static int imx8m_ddrc_probe(struct platform_device *pdev)
>   		return ret;
>   	}
>   
> -	ret = dev_pm_opp_of_add_table(dev);
> -	if (ret < 0) {
> -		dev_err(dev, "failed to get OPP table\n");
> -		return ret;
> -	}
> -
> -	ret = imx8m_ddrc_check_opps(dev);
> -	if (ret < 0)
> -		goto err;
> -
> +	priv->profile.polling_ms = 1000;

This change is not related to role of this patch.
Need to make the separate patch.

>   	priv->profile.target = imx8m_ddrc_target;
>   	priv->profile.exit = imx8m_ddrc_exit;
>   	priv->profile.get_cur_freq = imx8m_ddrc_get_cur_freq;
> @@ -427,13 +388,8 @@ static int imx8m_ddrc_probe(struct platform_device *pdev)
>   	if (IS_ERR(priv->devfreq)) {
>   		ret = PTR_ERR(priv->devfreq);
>   		dev_err(dev, "failed to add devfreq device: %d\n", ret);
> -		goto err;
>   	}
>   
> -	return 0;
> -
> -err:
> -	dev_pm_opp_of_remove_table(dev);
>   	return ret;
>   }
>   
>
Chanwoo Choi Sept. 15, 2021, 6:12 p.m. UTC | #2
On 21. 9. 15. 오후 12:29, Chanwoo Choi wrote:
> Hi,

> 

> OPP is mandatory for devfreq driver. Also, must need to add

> the OPP levels to  devicetree file, it is better to show

> the supported OPP list for the developer who don't know

> the detailed background of driver. If there are no any

> critical issue. I prefer the existing approach for the readability.


Also, by keeping the existing approach, the user is able to
select the their own OPP entries among the all supported frequencies
from EL3. Even if the some clock support the multiple frequencies,
the user might want to use the a few frequency instead of using
all supported frequencies.

> 

> On 21. 9. 14. 오전 2:38, Abel Vesa wrote:

>> i.MX8M platforms get their dram OPPs from the EL3.

>> We don't need to duplicate that in the kernel dram dts node.

>> We should just trust the OPPs provided by the EL3.

>>

>> Signed-off-by: Abel Vesa <abel.vesa@nxp.com>

>> ---

>>   drivers/devfreq/imx8m-ddrc.c | 50 +++---------------------------------

>>   1 file changed, 3 insertions(+), 47 deletions(-)

>>

>> diff --git a/drivers/devfreq/imx8m-ddrc.c b/drivers/devfreq/imx8m-ddrc.c

>> index 583123bf2100..f18a5c3c1c03 100644

>> --- a/drivers/devfreq/imx8m-ddrc.c

>> +++ b/drivers/devfreq/imx8m-ddrc.c

>> @@ -321,38 +321,9 @@ static int imx8m_ddrc_init_freq_info(struct 

>> device *dev)

>>           if (freq->dram_core_parent_index == 2 &&

>>                   freq->dram_alt_parent_index == 0)

>>               return -ENODEV;

>> -    }

>> -

>> -    return 0;

>> -}

>> -

>> -static int imx8m_ddrc_check_opps(struct device *dev)

>> -{

>> -    struct imx8m_ddrc *priv = dev_get_drvdata(dev);

>> -    struct imx8m_ddrc_freq *freq_info;

>> -    struct dev_pm_opp *opp;

>> -    unsigned long freq;

>> -    int i, opp_count;

>> -

>> -    /* Enumerate DT OPPs and disable those not supported by firmware */

>> -    opp_count = dev_pm_opp_get_opp_count(dev);

>> -    if (opp_count < 0)

>> -        return opp_count;

>> -    for (i = 0, freq = 0; i < opp_count; ++i, ++freq) {

>> -        opp = dev_pm_opp_find_freq_ceil(dev, &freq);

>> -        if (IS_ERR(opp)) {

>> -            dev_err(dev, "Failed enumerating OPPs: %ld\n",

>> -                PTR_ERR(opp));

>> -            return PTR_ERR(opp);

>> -        }

>> -        dev_pm_opp_put(opp);

>> -        freq_info = imx8m_ddrc_find_freq(priv, freq);

>> -        if (!freq_info) {

>> -            dev_info(dev, "Disable unsupported OPP %luHz %luMT/s\n",

>> -                    freq, DIV_ROUND_CLOSEST(freq, 250000));

>> -            dev_pm_opp_disable(dev, freq);

>> -        }

>> +        if (dev_pm_opp_add(dev, freq->rate * 250000, 0))

>> +            return -ENODEV;

>>       }

>>       return 0;

>> @@ -360,7 +331,6 @@ static int imx8m_ddrc_check_opps(struct device *dev)

>>   static void imx8m_ddrc_exit(struct device *dev)

>>   {

>> -    dev_pm_opp_of_remove_table(dev);

>>   }

>>   static int imx8m_ddrc_probe(struct platform_device *pdev)

>> @@ -407,16 +377,7 @@ static int imx8m_ddrc_probe(struct 

>> platform_device *pdev)

>>           return ret;

>>       }

>> -    ret = dev_pm_opp_of_add_table(dev);

>> -    if (ret < 0) {

>> -        dev_err(dev, "failed to get OPP table\n");

>> -        return ret;

>> -    }

>> -

>> -    ret = imx8m_ddrc_check_opps(dev);

>> -    if (ret < 0)

>> -        goto err;

>> -

>> +    priv->profile.polling_ms = 1000;

> 

> This change is not related to role of this patch.

> Need to make the separate patch.

> 

>>       priv->profile.target = imx8m_ddrc_target;

>>       priv->profile.exit = imx8m_ddrc_exit;

>>       priv->profile.get_cur_freq = imx8m_ddrc_get_cur_freq;

>> @@ -427,13 +388,8 @@ static int imx8m_ddrc_probe(struct 

>> platform_device *pdev)

>>       if (IS_ERR(priv->devfreq)) {

>>           ret = PTR_ERR(priv->devfreq);

>>           dev_err(dev, "failed to add devfreq device: %d\n", ret);

>> -        goto err;

>>       }

>> -    return 0;

>> -

>> -err:

>> -    dev_pm_opp_of_remove_table(dev);

>>       return ret;

>>   }

>>

> 

> 



-- 
Best Regards,
Samsung Electronics
Chanwoo Choi
diff mbox series

Patch

diff --git a/drivers/devfreq/imx8m-ddrc.c b/drivers/devfreq/imx8m-ddrc.c
index 583123bf2100..f18a5c3c1c03 100644
--- a/drivers/devfreq/imx8m-ddrc.c
+++ b/drivers/devfreq/imx8m-ddrc.c
@@ -321,38 +321,9 @@  static int imx8m_ddrc_init_freq_info(struct device *dev)
 		if (freq->dram_core_parent_index == 2 &&
 				freq->dram_alt_parent_index == 0)
 			return -ENODEV;
-	}
-
-	return 0;
-}
-
-static int imx8m_ddrc_check_opps(struct device *dev)
-{
-	struct imx8m_ddrc *priv = dev_get_drvdata(dev);
-	struct imx8m_ddrc_freq *freq_info;
-	struct dev_pm_opp *opp;
-	unsigned long freq;
-	int i, opp_count;
-
-	/* Enumerate DT OPPs and disable those not supported by firmware */
-	opp_count = dev_pm_opp_get_opp_count(dev);
-	if (opp_count < 0)
-		return opp_count;
-	for (i = 0, freq = 0; i < opp_count; ++i, ++freq) {
-		opp = dev_pm_opp_find_freq_ceil(dev, &freq);
-		if (IS_ERR(opp)) {
-			dev_err(dev, "Failed enumerating OPPs: %ld\n",
-				PTR_ERR(opp));
-			return PTR_ERR(opp);
-		}
-		dev_pm_opp_put(opp);
 
-		freq_info = imx8m_ddrc_find_freq(priv, freq);
-		if (!freq_info) {
-			dev_info(dev, "Disable unsupported OPP %luHz %luMT/s\n",
-					freq, DIV_ROUND_CLOSEST(freq, 250000));
-			dev_pm_opp_disable(dev, freq);
-		}
+		if (dev_pm_opp_add(dev, freq->rate * 250000, 0))
+			return -ENODEV;
 	}
 
 	return 0;
@@ -360,7 +331,6 @@  static int imx8m_ddrc_check_opps(struct device *dev)
 
 static void imx8m_ddrc_exit(struct device *dev)
 {
-	dev_pm_opp_of_remove_table(dev);
 }
 
 static int imx8m_ddrc_probe(struct platform_device *pdev)
@@ -407,16 +377,7 @@  static int imx8m_ddrc_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	ret = dev_pm_opp_of_add_table(dev);
-	if (ret < 0) {
-		dev_err(dev, "failed to get OPP table\n");
-		return ret;
-	}
-
-	ret = imx8m_ddrc_check_opps(dev);
-	if (ret < 0)
-		goto err;
-
+	priv->profile.polling_ms = 1000;
 	priv->profile.target = imx8m_ddrc_target;
 	priv->profile.exit = imx8m_ddrc_exit;
 	priv->profile.get_cur_freq = imx8m_ddrc_get_cur_freq;
@@ -427,13 +388,8 @@  static int imx8m_ddrc_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->devfreq)) {
 		ret = PTR_ERR(priv->devfreq);
 		dev_err(dev, "failed to add devfreq device: %d\n", ret);
-		goto err;
 	}
 
-	return 0;
-
-err:
-	dev_pm_opp_of_remove_table(dev);
 	return ret;
 }