diff mbox series

[3/4] mfd: db8500-prcmu: Get rid of cpufreq dependency

Message ID 20170810125221.25818-4-linus.walleij@linaro.org
State Superseded
Headers show
Series Switch Ux500 to use generic DT cpufreq | expand

Commit Message

Linus Walleij Aug. 10, 2017, 12:52 p.m. UTC 
The ARMSS clock, also known as the operating point of the
CPU, should not cross-depend on cpufreq like this. Move
the code to use just frequencies and remove the false
frequency (1GHz) and put in the actual frequency provided
by the ARMSS clock (998400000 Hz) as part of the process.

After this and the related cpufreq patch, the DB8500 will
simply use the standard DT cpufreq driver to change the
operating points through the common clock framework using
the ARMSS clock.

Cc: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
Lee, this needs to be merged together with the other
related cpufreq patches, so an ACK to take this through
the cpufreq subsystem would be appreciated once you're
pleased with it.
---
 drivers/mfd/db8500-prcmu.c | 59 +++++++++++++---------------------------------
 1 file changed, 17 insertions(+), 42 deletions(-)

-- 
2.9.4

Comments

Lee Jones Aug. 14, 2017, 7:07 a.m. UTC | #1 
On Thu, 10 Aug 2017, Linus Walleij wrote:

> The ARMSS clock, also known as the operating point of the

> CPU, should not cross-depend on cpufreq like this. Move

> the code to use just frequencies and remove the false

> frequency (1GHz) and put in the actual frequency provided

> by the ARMSS clock (998400000 Hz) as part of the process.

> 

> After this and the related cpufreq patch, the DB8500 will

> simply use the standard DT cpufreq driver to change the

> operating points through the common clock framework using

> the ARMSS clock.

> 

> Cc: Lee Jones <lee.jones@linaro.org>

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> ---

> Lee, this needs to be merged together with the other

> related cpufreq patches, so an ACK to take this through

> the cpufreq subsystem would be appreciated once you're

> pleased with it.

> ---

>  drivers/mfd/db8500-prcmu.c | 59 +++++++++++++---------------------------------

>  1 file changed, 17 insertions(+), 42 deletions(-)


Make sure this passes checkpatch.pl.

> diff --git a/drivers/mfd/db8500-prcmu.c b/drivers/mfd/db8500-prcmu.c

> index 5c739ac752e8..a66be0203ece 100644

> --- a/drivers/mfd/db8500-prcmu.c

> +++ b/drivers/mfd/db8500-prcmu.c

> @@ -33,7 +33,6 @@

>  #include <linux/mfd/abx500/ab8500.h>

>  #include <linux/regulator/db8500-prcmu.h>

>  #include <linux/regulator/machine.h>

> -#include <linux/cpufreq.h>

>  #include <linux/platform_data/ux500_wdt.h>

>  #include <linux/platform_data/db8500_thermal.h>

>  #include "dbx500-prcmu-regs.h"

> @@ -1692,32 +1691,22 @@ static long round_clock_rate(u8 clock, unsigned long rate)

>  	return rounded_rate;

>  }

>  

> -/* CPU FREQ table, may be changed due to if MAX_OPP is supported. */

> -static struct cpufreq_frequency_table db8500_cpufreq_table[] = {

> -	{ .frequency = 200000, .driver_data = ARM_EXTCLK,},

> -	{ .frequency = 400000, .driver_data = ARM_50_OPP,},

> -	{ .frequency = 800000, .driver_data = ARM_100_OPP,},

> -	{ .frequency = CPUFREQ_TABLE_END,}, /* To be used for MAX_OPP. */

> -	{ .frequency = CPUFREQ_TABLE_END,},

> -};

> +static const unsigned long armss_freqs[] = { 200000000, 400000000, 800000000, 998400000 };

>  

>  static long round_armss_rate(unsigned long rate)

>  {

> -	struct cpufreq_frequency_table *pos;

> -	long freq = 0;

> -

> -	/* cpufreq table frequencies is in KHz. */

> -	rate = rate / 1000;

> +	unsigned long freq = 0;

> +	int i;

>  

>  	/* Find the corresponding arm opp from the cpufreq table. */

> -	cpufreq_for_each_entry(pos, db8500_cpufreq_table) {

> -		freq = pos->frequency;

> -		if (freq == rate)

> +	for (i = 0; i < ARRAY_SIZE(armss_freqs); i++) {

> +		freq = armss_freqs[i];

> +		if (rate <= freq)

>  			break;

>  	}

>  

>  	/* Return the last valid value, even if a match was not found. */

> -	return freq * 1000;

> +	return freq;

>  }

>  

>  #define MIN_PLL_VCO_RATE 600000000ULL

> @@ -1854,21 +1843,23 @@ static void set_clock_rate(u8 clock, unsigned long rate)

>  

>  static int set_armss_rate(unsigned long rate)

>  {

> -	struct cpufreq_frequency_table *pos;

> -

> -	/* cpufreq table frequencies is in KHz. */

> -	rate = rate / 1000;

> +	unsigned long freq;

> +	u8 opps[] = { ARM_EXTCLK, ARM_50_OPP, ARM_100_OPP, ARM_MAX_OPP };

> +	int i;

>  

>  	/* Find the corresponding arm opp from the cpufreq table. */

> -	cpufreq_for_each_entry(pos, db8500_cpufreq_table)

> -		if (pos->frequency == rate)

> +	for (i = 0; i < ARRAY_SIZE(armss_freqs); i++) {

> +		freq = armss_freqs[i];

> +		if (rate == freq)

>  			break;

> +	}


Why don't you just call round_armss_rate()?

> -	if (pos->frequency != rate)

> +	if (rate != freq)

>  		return -EINVAL;

>  

>  	/* Set the new arm opp. */

> -	return db8500_prcmu_set_arm_opp(pos->driver_data);

> +	pr_debug("SET ARM OPP 0x%02x\n", opps[i]);


Add a '\n' here.

Once fixed please apply my:

For my own reference:
  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Linus Walleij Aug. 16, 2017, 8:16 a.m. UTC | #2 
On Mon, Aug 14, 2017 at 9:07 AM, Lee Jones <lee.jones@linaro.org> wrote:
> On Thu, 10 Aug 2017, Linus Walleij wrote:

>

>> The ARMSS clock, also known as the operating point of the

>> CPU, should not cross-depend on cpufreq like this. Move

>> the code to use just frequencies and remove the false

>> frequency (1GHz) and put in the actual frequency provided

>> by the ARMSS clock (998400000 Hz) as part of the process.

>>

>> After this and the related cpufreq patch, the DB8500 will

>> simply use the standard DT cpufreq driver to change the

>> operating points through the common clock framework using

>> the ARMSS clock.

>>

>> Cc: Lee Jones <lee.jones@linaro.org>

>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>


>>  drivers/mfd/db8500-prcmu.c | 59 +++++++++++++---------------------------------

>>  1 file changed, 17 insertions(+), 42 deletions(-)

>

> Make sure this passes checkpatch.pl.


Sure thing :)
$ scripts/checkpatch.pl
cpufreq/0003-mfd-db8500-prcmu-Get-rid-of-cpufreq-dependency.patch
total: 0 errors, 0 warnings, 112 lines checked

cpufreq/0003-mfd-db8500-prcmu-Get-rid-of-cpufreq-dependency.patch has
no obvious style problems and is ready for submission.

>> +     int i;

>>

>>       /* Find the corresponding arm opp from the cpufreq table. */

>> -     cpufreq_for_each_entry(pos, db8500_cpufreq_table)

>> -             if (pos->frequency == rate)

>> +     for (i = 0; i < ARRAY_SIZE(armss_freqs); i++) {

>> +             freq = armss_freqs[i];

>> +             if (rate == freq)

>>                       break;

>> +     }

>

> Why don't you just call round_armss_rate()?


It's because I need the index of the located frequency (i), and the other
function due to being called from the clock framework has to have
that simple signature.

> Once fixed please apply my:

>

> For my own reference:

>   Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>


OK thanks!

Yours,
Linus Walleij
Lee Jones Aug. 16, 2017, 11:27 a.m. UTC | #3 
On Wed, 16 Aug 2017, Linus Walleij wrote:

> On Mon, Aug 14, 2017 at 9:07 AM, Lee Jones <lee.jones@linaro.org> wrote:

> > On Thu, 10 Aug 2017, Linus Walleij wrote:

> >

> >> The ARMSS clock, also known as the operating point of the

> >> CPU, should not cross-depend on cpufreq like this. Move

> >> the code to use just frequencies and remove the false

> >> frequency (1GHz) and put in the actual frequency provided

> >> by the ARMSS clock (998400000 Hz) as part of the process.

> >>

> >> After this and the related cpufreq patch, the DB8500 will

> >> simply use the standard DT cpufreq driver to change the

> >> operating points through the common clock framework using

> >> the ARMSS clock.

> >>

> >> Cc: Lee Jones <lee.jones@linaro.org>

> >> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> 

> >>  drivers/mfd/db8500-prcmu.c | 59 +++++++++++++---------------------------------

> >>  1 file changed, 17 insertions(+), 42 deletions(-)

> >

> > Make sure this passes checkpatch.pl.

> 

> Sure thing :)

> $ scripts/checkpatch.pl

> cpufreq/0003-mfd-db8500-prcmu-Get-rid-of-cpufreq-dependency.patch

> total: 0 errors, 0 warnings, 112 lines checked

> 

> cpufreq/0003-mfd-db8500-prcmu-Get-rid-of-cpufreq-dependency.patch has

> no obvious style problems and is ready for submission.

>

Strange, this line looks way over 80 chars:

static const unsigned long armss_freqs[] = { 200000000, 400000000, 800000000, 998400000 };

Although when I look at it in Gmail, you appear to have a '\n' in
here, very odd.

Anyway, even if the '\n' is in there, please format it like:

static const unsigned long armss_freqs[] = {
	200000000,
	400000000,
	800000000,
	998400000
};


> >> +     int i;

> >>

> >>       /* Find the corresponding arm opp from the cpufreq table. */

> >> -     cpufreq_for_each_entry(pos, db8500_cpufreq_table)

> >> -             if (pos->frequency == rate)

> >> +     for (i = 0; i < ARRAY_SIZE(armss_freqs); i++) {

> >> +             freq = armss_freqs[i];

> >> +             if (rate == freq)

> >>                       break;

> >> +     }

> >

> > Why don't you just call round_armss_rate()?

> 

> It's because I need the index of the located frequency (i), and the other

> function due to being called from the clock framework has to have

> that simple signature.

> 

> > Once fixed please apply my:

> >

> > For my own reference:

> >   Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

> 

> OK thanks!

> 

> Yours,

> Linus Walleij


-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
diff mbox series

Patch

diff --git a/drivers/mfd/db8500-prcmu.c b/drivers/mfd/db8500-prcmu.c
index 5c739ac752e8..a66be0203ece 100644
--- a/drivers/mfd/db8500-prcmu.c
+++ b/drivers/mfd/db8500-prcmu.c
@@ -33,7 +33,6 @@ 
 #include <linux/mfd/abx500/ab8500.h>
 #include <linux/regulator/db8500-prcmu.h>
 #include <linux/regulator/machine.h>
-#include <linux/cpufreq.h>
 #include <linux/platform_data/ux500_wdt.h>
 #include <linux/platform_data/db8500_thermal.h>
 #include "dbx500-prcmu-regs.h"
@@ -1692,32 +1691,22 @@  static long round_clock_rate(u8 clock, unsigned long rate)
 	return rounded_rate;
 }
 
-/* CPU FREQ table, may be changed due to if MAX_OPP is supported. */
-static struct cpufreq_frequency_table db8500_cpufreq_table[] = {
-	{ .frequency = 200000, .driver_data = ARM_EXTCLK,},
-	{ .frequency = 400000, .driver_data = ARM_50_OPP,},
-	{ .frequency = 800000, .driver_data = ARM_100_OPP,},
-	{ .frequency = CPUFREQ_TABLE_END,}, /* To be used for MAX_OPP. */
-	{ .frequency = CPUFREQ_TABLE_END,},
-};
+static const unsigned long armss_freqs[] = { 200000000, 400000000, 800000000, 998400000 };
 
 static long round_armss_rate(unsigned long rate)
 {
-	struct cpufreq_frequency_table *pos;
-	long freq = 0;
-
-	/* cpufreq table frequencies is in KHz. */
-	rate = rate / 1000;
+	unsigned long freq = 0;
+	int i;
 
 	/* Find the corresponding arm opp from the cpufreq table. */
-	cpufreq_for_each_entry(pos, db8500_cpufreq_table) {
-		freq = pos->frequency;
-		if (freq == rate)
+	for (i = 0; i < ARRAY_SIZE(armss_freqs); i++) {
+		freq = armss_freqs[i];
+		if (rate <= freq)
 			break;
 	}
 
 	/* Return the last valid value, even if a match was not found. */
-	return freq * 1000;
+	return freq;
 }
 
 #define MIN_PLL_VCO_RATE 600000000ULL
@@ -1854,21 +1843,23 @@  static void set_clock_rate(u8 clock, unsigned long rate)
 
 static int set_armss_rate(unsigned long rate)
 {
-	struct cpufreq_frequency_table *pos;
-
-	/* cpufreq table frequencies is in KHz. */
-	rate = rate / 1000;
+	unsigned long freq;
+	u8 opps[] = { ARM_EXTCLK, ARM_50_OPP, ARM_100_OPP, ARM_MAX_OPP };
+	int i;
 
 	/* Find the corresponding arm opp from the cpufreq table. */
-	cpufreq_for_each_entry(pos, db8500_cpufreq_table)
-		if (pos->frequency == rate)
+	for (i = 0; i < ARRAY_SIZE(armss_freqs); i++) {
+		freq = armss_freqs[i];
+		if (rate == freq)
 			break;
+	}
 
-	if (pos->frequency != rate)
+	if (rate != freq)
 		return -EINVAL;
 
 	/* Set the new arm opp. */
-	return db8500_prcmu_set_arm_opp(pos->driver_data);
+	pr_debug("SET ARM OPP 0x%02x\n", opps[i]);
+	return db8500_prcmu_set_arm_opp(opps[i]);
 }
 
 static int set_plldsi_rate(unsigned long rate)
@@ -3049,12 +3040,6 @@  static const struct mfd_cell db8500_prcmu_devs[] = {
 		.pdata_size = sizeof(db8500_regulators),
 	},
 	{
-		.name = "cpufreq-ux500",
-		.of_compatible = "stericsson,cpufreq-ux500",
-		.platform_data = &db8500_cpufreq_table,
-		.pdata_size = sizeof(db8500_cpufreq_table),
-	},
-	{
 		.name = "cpuidle-dbx500",
 		.of_compatible = "stericsson,cpuidle-dbx500",
 	},
@@ -3067,14 +3052,6 @@  static const struct mfd_cell db8500_prcmu_devs[] = {
 	},
 };
 
-static void db8500_prcmu_update_cpufreq(void)
-{
-	if (prcmu_has_arm_maxopp()) {
-		db8500_cpufreq_table[3].frequency = 1000000;
-		db8500_cpufreq_table[3].driver_data = ARM_MAX_OPP;
-	}
-}
-
 static int db8500_prcmu_register_ab8500(struct device *parent)
 {
 	struct device_node *np;
@@ -3160,8 +3137,6 @@  static int db8500_prcmu_probe(struct platform_device *pdev)
 
 	prcmu_config_esram0_deep_sleep(ESRAM0_DEEP_SLEEP_STATE_RET);
 
-	db8500_prcmu_update_cpufreq();
-
 	err = mfd_add_devices(&pdev->dev, 0, common_prcmu_devs,
 			      ARRAY_SIZE(common_prcmu_devs), NULL, 0, db8500_irq_domain);
 	if (err) {