subject:"\[PATCH v1\] cpufreq\: qcom\: Read voltage LUT and populate OPP"

Re: [PATCH v1] cpufreq: qcom: Read voltage LUT and populate OPP

2019-01-09 Thread Taniya Das





On 1/8/2019 5:32 AM, Matthias Kaehlcke wrote:

Hi Taniya.

On Mon, Dec 24, 2018 at 12:29:18AM +0530, Taniya Das wrote:


Could you help validating with the patch below?


...


diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c
b/drivers/cpufreq/qcom-cpufreq-hw.c
index 7559b87..23338b2 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -81,7 +81,6 @@ static int qcom_cpufreq_hw_read_lut(struct device *dev,
 u32 volt;
 unsigned int max_cores = cpumask_weight(policy->cpus);
 struct cpufreq_frequency_table  *table;
-   unsigned long cpu_r;

 table = kcalloc(LUT_MAX_ENTRIES + 1, sizeof(*table), GFP_KERNEL);
 if (!table)
@@ -110,6 +109,8 @@ static int qcom_cpufreq_hw_read_lut(struct device *dev,
 table[i].frequency = freq;
 dev_dbg(dev, "index=%d freq=%d, core_count %d\n", i,
 freq, core_count);
+   dev_pm_opp_add(get_cpu_device(policy->cpu),
+   freq * 1000, volt);
 }

 /*
@@ -126,6 +127,8 @@ static int qcom_cpufreq_hw_read_lut(struct device *dev,
 if (prev_cc != max_cores) {
 prev->frequency = prev_freq;
 prev->flags = CPUFREQ_BOOST_FREQ;
+   dev_pm_opp_add(get_cpu_device(policy->cpu),
+   prev_freq * 1000, volt);
 }

 break;
@@ -133,12 +136,9 @@ static int qcom_cpufreq_hw_read_lut(struct device *dev,

 prev_cc = core_count;
 prev_freq = freq;
-
-   freq *= 1000;
-   for_each_cpu(cpu_r, policy->cpus)
-   dev_pm_opp_add(get_cpu_device(cpu_r), freq, volt);
 }

+   dev_pm_opp_set_sharing_cpus(get_cpu_device(policy->cpu),
policy->cpus);
 table[i].frequency = CPUFREQ_TABLE_END;
 policy->freq_table = table;

@@ -245,6 +245,7 @@ static int qcom_cpufreq_hw_cpu_exit(struct
cpufreq_policy *policy)
  {
 void __iomem *base = policy->driver_data - REG_PERF_STATE;

+   dev_pm_opp_cpumask_remove_table(policy->cpus);


Evan found that this doesn't actually remove dynamically added
OPPs. You'll have to use the shiny new dev_pm_opp_remove_all_dynamic()
(https://lore.kernel.org/patchwork/patch/1028942/) instead.

Cheers

Matthias



Thanks, updated the next patch to use the new API.

--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

--

Re: [PATCH v1] cpufreq: qcom: Read voltage LUT and populate OPP

2019-01-07 Thread Matthias Kaehlcke

Hi Taniya. 

On Mon, Dec 24, 2018 at 12:29:18AM +0530, Taniya Das wrote:

> Could you help validating with the patch below?

...

> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c
> b/drivers/cpufreq/qcom-cpufreq-hw.c
> index 7559b87..23338b2 100644
> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -81,7 +81,6 @@ static int qcom_cpufreq_hw_read_lut(struct device *dev,
> u32 volt;
> unsigned int max_cores = cpumask_weight(policy->cpus);
> struct cpufreq_frequency_table  *table;
> -   unsigned long cpu_r;
> 
> table = kcalloc(LUT_MAX_ENTRIES + 1, sizeof(*table), GFP_KERNEL);
> if (!table)
> @@ -110,6 +109,8 @@ static int qcom_cpufreq_hw_read_lut(struct device *dev,
> table[i].frequency = freq;
> dev_dbg(dev, "index=%d freq=%d, core_count %d\n", i,
> freq, core_count);
> +   dev_pm_opp_add(get_cpu_device(policy->cpu),
> +   freq * 1000, volt);
> }
> 
> /*
> @@ -126,6 +127,8 @@ static int qcom_cpufreq_hw_read_lut(struct device *dev,
> if (prev_cc != max_cores) {
> prev->frequency = prev_freq;
> prev->flags = CPUFREQ_BOOST_FREQ;
> +   dev_pm_opp_add(get_cpu_device(policy->cpu),
> +   prev_freq * 1000, volt);
> }
> 
> break;
> @@ -133,12 +136,9 @@ static int qcom_cpufreq_hw_read_lut(struct device *dev,
> 
> prev_cc = core_count;
> prev_freq = freq;
> -
> -   freq *= 1000;
> -   for_each_cpu(cpu_r, policy->cpus)
> -   dev_pm_opp_add(get_cpu_device(cpu_r), freq, volt);
> }
> 
> +   dev_pm_opp_set_sharing_cpus(get_cpu_device(policy->cpu),
> policy->cpus);
> table[i].frequency = CPUFREQ_TABLE_END;
> policy->freq_table = table;
> 
> @@ -245,6 +245,7 @@ static int qcom_cpufreq_hw_cpu_exit(struct
> cpufreq_policy *policy)
>  {
> void __iomem *base = policy->driver_data - REG_PERF_STATE;
> 
> +   dev_pm_opp_cpumask_remove_table(policy->cpus);

Evan found that this doesn't actually remove dynamically added
OPPs. You'll have to use the shiny new dev_pm_opp_remove_all_dynamic()
(https://lore.kernel.org/patchwork/patch/1028942/) instead.

Cheers

Matthias

Re: [PATCH v1] cpufreq: qcom: Read voltage LUT and populate OPP

2019-01-06 Thread Taniya Das





On 12/22/2018 3:15 AM, Stephen Boyd wrote:

Quoting Taniya Das (2018-12-21 10:06:48)

Add support to read the voltage look up table and populate OPP for all
corresponding CPUS.


Yes, but why? Please specify the motivations in the commit text.


Sure, would update in the next patch.

--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

--

Re: [PATCH v1] cpufreq: qcom: Read voltage LUT and populate OPP

2019-01-06 Thread Taniya Das





On 12/27/2018 1:02 AM, Matthias Kaehlcke wrote:

Hi Taniya,

On Mon, Dec 24, 2018 at 12:29:18AM +0530, Taniya Das wrote:

Hello Matthias,

Thanks for your review comments.

On 12/22/2018 2:27 AM, Matthias Kaehlcke wrote:

Hi Taniya,

On Fri, Dec 21, 2018 at 11:36:48PM +0530, Taniya Das wrote:

Add support to read the voltage look up table and populate OPP for all
corresponding CPUS.

Signed-off-by: Taniya Das 
---
   drivers/cpufreq/qcom-cpufreq-hw.c | 32 ++--
   1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c 
b/drivers/cpufreq/qcom-cpufreq-hw.c
index d83939a..7559b87 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -10,18 +10,21 @@
   #include 
   #include 
   #include 
+#include 
   #include 

   #define LUT_MAX_ENTRIES  40U
   #define LUT_SRC  GENMASK(31, 30)
   #define LUT_L_VALGENMASK(7, 0)
   #define LUT_CORE_COUNT   GENMASK(18, 16)
+#define LUT_VOLT   GENMASK(11, 0)
   #define LUT_ROW_SIZE 32
   #define CLK_HW_DIV   2

   /* Register offsets */
   #define REG_ENABLE   0x0
-#define REG_LUT_TABLE  0x110
+#define REG_FREQ_LUT_TABLE 0x110
+#define REG_VOLT_LUT_TABLE 0x114


The new names suggest that there is a LUT for frequencies and another
one for voltages. I don't have access to hardware documentation, but
from the code and offsets in this driver it seems there is a single
table at offset 0x110, with a 'row' of 32 bytes per OPP. Within this
row the frequency (and other values) is located at offset 0, the
voltage at offset 4.

I'd suggest to keep REG_LUT_TABLE, add a define LUT_OFFSET_VOLTAGE/MV
(or similar) and adjust the math in qcom_cpufreq_hw_read_lut() to use

  > > REG_LUT_TABLE as base offset.




These names are as per HW documentation and the math is kept as per the
documentation for reading the voltage.


The HW documentation is confusing then and I'm not convinced this
should be carried over 1:1 to the driver. In any case this
documentation is only available to a reduced audience, why make it
harder for everyone else?

I think something like this would be preferable (removed _TABLE suffix,
since that's already part of LUT):

#define OFFSET_LUT  0x110
#define REG_FREQ_LUT0x00
#define REG_VOLT_LUT0x04



Sorry :( ,This is not the correct interpretation as per the 
Documentation. I would leave it as it is. Though I could update the 
macro names.



freq = read(OFFSET_LUT + (LUT_ROW_SIZE * i) + REG_FREQ_LUT);
volt = read(OFFSET_LUT + (LUT_ROW_SIZE * i) + REG_VOLT_LUT);

or probably better:

row_addr = OFFSET_LUT + (LUT_ROW_SIZE * i);
freq = read(row_addr + REG_FREQ_LUT);
volt = read(row_addr + REG_VOLT_LUT);


   #define REG_PERF_STATE   0x920

   static unsigned long cpu_hw_rate, xo_rate;
@@ -75,19 +78,26 @@ static int qcom_cpufreq_hw_read_lut(struct device *dev,
void __iomem *base)
   {
u32 data, src, lval, i, core_count, prev_cc = 0, prev_freq = 0, freq;
+   u32 volt;
unsigned int max_cores = cpumask_weight(policy->cpus);
struct cpufreq_frequency_table  *table;
+   unsigned long cpu_r;


nit: why 'cpu_r' and not just 'cpu'?

(if it is needed at all, see my comment below)



Sure, will update it to 'cpu'.



table = kcalloc(LUT_MAX_ENTRIES + 1, sizeof(*table), GFP_KERNEL);
if (!table)
return -ENOMEM;

for (i = 0; i < LUT_MAX_ENTRIES; i++) {
-   data = readl_relaxed(base + REG_LUT_TABLE + i * LUT_ROW_SIZE);
+   data = readl_relaxed(base + REG_FREQ_LUT_TABLE +
+ i * LUT_ROW_SIZE);
src = FIELD_GET(LUT_SRC, data);
lval = FIELD_GET(LUT_L_VAL, data);
core_count = FIELD_GET(LUT_CORE_COUNT, data);

+   data = readl_relaxed(base + REG_VOLT_LUT_TABLE +
+ i * LUT_ROW_SIZE);
+   volt = FIELD_GET(LUT_VOLT, data) * 1000;
+
if (src)
freq = xo_rate * lval / 1000;
else
@@ -123,6 +133,10 @@ static int qcom_cpufreq_hw_read_lut(struct device *dev,

prev_cc = core_count;
prev_freq = freq;
+
+   freq *= 1000;
+   for_each_cpu(cpu_r, policy->cpus)
+   dev_pm_opp_add(get_cpu_device(cpu_r), freq, volt);


Are you sure we want to duplicate the OPP entries for all CPUs in the
cluster? IIUC the frequencies of the cores in a cluster can't be
changed individually, hence the cores should have a shared table. I
think dev_pm_opp_get_sharing_cpus() does what you need.

You currently also add OPPs for invalid frequencies. From my SDM845
device:

cat

Re: [PATCH v1] cpufreq: qcom: Read voltage LUT and populate OPP

2018-12-26 Thread Matthias Kaehlcke

Hi Taniya,

On Mon, Dec 24, 2018 at 12:29:18AM +0530, Taniya Das wrote:
> Hello Matthias,
> 
> Thanks for your review comments.
> 
> On 12/22/2018 2:27 AM, Matthias Kaehlcke wrote:
> > Hi Taniya,
> > 
> > On Fri, Dec 21, 2018 at 11:36:48PM +0530, Taniya Das wrote:
> > > Add support to read the voltage look up table and populate OPP for all
> > > corresponding CPUS.
> > > 
> > > Signed-off-by: Taniya Das 
> > > ---
> > >   drivers/cpufreq/qcom-cpufreq-hw.c | 32 ++--
> > >   1 file changed, 30 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c 
> > > b/drivers/cpufreq/qcom-cpufreq-hw.c
> > > index d83939a..7559b87 100644
> > > --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> > > +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> > > @@ -10,18 +10,21 @@
> > >   #include 
> > >   #include 
> > >   #include 
> > > +#include 
> > >   #include 
> > > 
> > >   #define LUT_MAX_ENTRIES 40U
> > >   #define LUT_SRC GENMASK(31, 30)
> > >   #define LUT_L_VAL   GENMASK(7, 0)
> > >   #define LUT_CORE_COUNT  GENMASK(18, 16)
> > > +#define LUT_VOLT GENMASK(11, 0)
> > >   #define LUT_ROW_SIZE32
> > >   #define CLK_HW_DIV  2
> > > 
> > >   /* Register offsets */
> > >   #define REG_ENABLE  0x0
> > > -#define REG_LUT_TABLE0x110
> > > +#define REG_FREQ_LUT_TABLE   0x110
> > > +#define REG_VOLT_LUT_TABLE   0x114
> > 
> > The new names suggest that there is a LUT for frequencies and another
> > one for voltages. I don't have access to hardware documentation, but
> > from the code and offsets in this driver it seems there is a single
> > table at offset 0x110, with a 'row' of 32 bytes per OPP. Within this
> > row the frequency (and other values) is located at offset 0, the
> > voltage at offset 4.
> > 
> > I'd suggest to keep REG_LUT_TABLE, add a define LUT_OFFSET_VOLTAGE/MV
> > (or similar) and adjust the math in qcom_cpufreq_hw_read_lut() to use
 > > REG_LUT_TABLE as base offset.
> > 
> 
> These names are as per HW documentation and the math is kept as per the
> documentation for reading the voltage.

The HW documentation is confusing then and I'm not convinced this
should be carried over 1:1 to the driver. In any case this
documentation is only available to a reduced audience, why make it
harder for everyone else?

I think something like this would be preferable (removed _TABLE suffix,
since that's already part of LUT):

#define OFFSET_LUT  0x110
#define REG_FREQ_LUT0x00
#define REG_VOLT_LUT0x04

freq = read(OFFSET_LUT + (LUT_ROW_SIZE * i) + REG_FREQ_LUT);
volt = read(OFFSET_LUT + (LUT_ROW_SIZE * i) + REG_VOLT_LUT);

or probably better:

row_addr = OFFSET_LUT + (LUT_ROW_SIZE * i);
freq = read(row_addr + REG_FREQ_LUT);
volt = read(row_addr + REG_VOLT_LUT);

> > >   #define REG_PERF_STATE  0x920
> > > 
> > >   static unsigned long cpu_hw_rate, xo_rate;
> > > @@ -75,19 +78,26 @@ static int qcom_cpufreq_hw_read_lut(struct device 
> > > *dev,
> > >   void __iomem *base)
> > >   {
> > >   u32 data, src, lval, i, core_count, prev_cc = 0, prev_freq = 0, 
> > > freq;
> > > + u32 volt;
> > >   unsigned int max_cores = cpumask_weight(policy->cpus);
> > >   struct cpufreq_frequency_table  *table;
> > > + unsigned long cpu_r;
> > 
> > nit: why 'cpu_r' and not just 'cpu'?
> > 
> > (if it is needed at all, see my comment below)
> > 
> > > 
> > >   table = kcalloc(LUT_MAX_ENTRIES + 1, sizeof(*table), 
> > > GFP_KERNEL);
> > >   if (!table)
> > >   return -ENOMEM;
> > > 
> > >   for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> > > - data = readl_relaxed(base + REG_LUT_TABLE + i * LUT_ROW_SIZE);
> > > + data = readl_relaxed(base + REG_FREQ_LUT_TABLE +
> > > +   i * LUT_ROW_SIZE);
> > >   src = FIELD_GET(LUT_SRC, data);
> > >   lval = FIELD_GET(LUT_L_VAL, data);
> > >   core_count = FIELD_GET(LUT_CORE_COUNT, data);
> > > 
> > > + data = readl_relaxed(base + REG_VOLT_LUT_TABLE +
> > > +   i * LUT_ROW_SIZE);
> > > + volt = FIELD_GET(LUT_VOLT, data) * 1000;
> > > +
> > >   if (src)
> > >   freq = xo_rate * lval / 1000;
> > >   else
> > > @@ -123,6 +133,10 @@ static int qcom_cpufreq_hw_read_lut(struct device 
> > > *dev,
> > > 
> > >   prev_cc = core_count;
> > >   prev_freq = freq;
> > > +
> > > + freq *= 1000;
> > > + for_each_cpu(cpu_r, policy->cpus)
> > > + dev_pm_opp_add(get_cpu_device(cpu_r), freq, volt);
> > 
> > Are you sure we want to duplicate the OPP entries for all CPUs in the
> > cluster? IIUC the

Re: [PATCH v1] cpufreq: qcom: Read voltage LUT and populate OPP

2018-12-23 Thread Taniya Das


Hello Matthias,

Thanks for your review comments.

On 12/22/2018 2:27 AM, Matthias Kaehlcke wrote:

Hi Taniya,

On Fri, Dec 21, 2018 at 11:36:48PM +0530, Taniya Das wrote:

Add support to read the voltage look up table and populate OPP for all
corresponding CPUS.

Signed-off-by: Taniya Das 
---
  drivers/cpufreq/qcom-cpufreq-hw.c | 32 ++--
  1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c 
b/drivers/cpufreq/qcom-cpufreq-hw.c
index d83939a..7559b87 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -10,18 +10,21 @@
  #include 
  #include 
  #include 
+#include 
  #include 

  #define LUT_MAX_ENTRIES   40U
  #define LUT_SRC   GENMASK(31, 30)
  #define LUT_L_VAL GENMASK(7, 0)
  #define LUT_CORE_COUNTGENMASK(18, 16)
+#define LUT_VOLT   GENMASK(11, 0)
  #define LUT_ROW_SIZE  32
  #define CLK_HW_DIV2

  /* Register offsets */
  #define REG_ENABLE0x0
-#define REG_LUT_TABLE  0x110
+#define REG_FREQ_LUT_TABLE 0x110
+#define REG_VOLT_LUT_TABLE 0x114


The new names suggest that there is a LUT for frequencies and another
one for voltages. I don't have access to hardware documentation, but
from the code and offsets in this driver it seems there is a single
table at offset 0x110, with a 'row' of 32 bytes per OPP. Within this
row the frequency (and other values) is located at offset 0, the
voltage at offset 4.

I'd suggest to keep REG_LUT_TABLE, add a define LUT_OFFSET_VOLTAGE/MV
(or similar) and adjust the math in qcom_cpufreq_hw_read_lut() to use
REG_LUT_TABLE as base offset.



These names are as per HW documentation and the math is kept as per the 
documentation for reading the voltage.



  #define REG_PERF_STATE0x920

  static unsigned long cpu_hw_rate, xo_rate;
@@ -75,19 +78,26 @@ static int qcom_cpufreq_hw_read_lut(struct device *dev,
void __iomem *base)
  {
u32 data, src, lval, i, core_count, prev_cc = 0, prev_freq = 0, freq;
+   u32 volt;
unsigned int max_cores = cpumask_weight(policy->cpus);
struct cpufreq_frequency_table  *table;
+   unsigned long cpu_r;


nit: why 'cpu_r' and not just 'cpu'?

(if it is needed at all, see my comment below)



table = kcalloc(LUT_MAX_ENTRIES + 1, sizeof(*table), GFP_KERNEL);
if (!table)
return -ENOMEM;

for (i = 0; i < LUT_MAX_ENTRIES; i++) {
-   data = readl_relaxed(base + REG_LUT_TABLE + i * LUT_ROW_SIZE);
+   data = readl_relaxed(base + REG_FREQ_LUT_TABLE +
+ i * LUT_ROW_SIZE);
src = FIELD_GET(LUT_SRC, data);
lval = FIELD_GET(LUT_L_VAL, data);
core_count = FIELD_GET(LUT_CORE_COUNT, data);

+   data = readl_relaxed(base + REG_VOLT_LUT_TABLE +
+ i * LUT_ROW_SIZE);
+   volt = FIELD_GET(LUT_VOLT, data) * 1000;
+
if (src)
freq = xo_rate * lval / 1000;
else
@@ -123,6 +133,10 @@ static int qcom_cpufreq_hw_read_lut(struct device *dev,

prev_cc = core_count;
prev_freq = freq;
+
+   freq *= 1000;
+   for_each_cpu(cpu_r, policy->cpus)
+   dev_pm_opp_add(get_cpu_device(cpu_r), freq, volt);


Are you sure we want to duplicate the OPP entries for all CPUs in the
cluster? IIUC the frequencies of the cores in a cluster can't be
changed individually, hence the cores should have a shared table. I
think dev_pm_opp_get_sharing_cpus() does what you need.

You currently also add OPPs for invalid frequencies. From my SDM845
device:

cat /sys/devices/system/cpu/cpufreq/policy4/scaling_available_freq
   => 825600 902400 979200 1056000 1209600 1286400 1363200 1459200
   1536000 1612800 1689600 1766400 1843200 192 1996800 2092800
   2169600 2246400 2323200 240 2476800 2553600 2649600

cat /sys/devices/system/cpu/cpufreq/policy4/scaling_boost_frequencies
2803200

ls /sys/kernel/debug/opp/cpu4/
opp:105600  opp:161280  opp:209280  opp:255360  opp:82560
opp:120960  opp:168960  opp:216960  opp:264960  opp:90240
opp:128640  opp:176640  opp:224640  opp:270720  opp:97920
opp:136320  opp:184320  opp:232320  opp:276480
opp:145920  opp:192000  opp:24  opp:278400
opp:153600  opp:199680  opp:247680  opp:280320

There are OPP entries for 270720, 276480 and 278400 Hz,
however these frequencies appear neither in available_frequencies nor
boost_frequencies.


}



Could you help validating with the patch below?

Re: [PATCH v1] cpufreq: qcom: Read voltage LUT and populate OPP

2018-12-21 Thread Stephen Boyd

Quoting Taniya Das (2018-12-21 10:06:48)
> Add support to read the voltage look up table and populate OPP for all
> corresponding CPUS.

Yes, but why? Please specify the motivations in the commit text.

Re: [PATCH v1] cpufreq: qcom: Read voltage LUT and populate OPP

2018-12-21 Thread Matthias Kaehlcke

Hi Taniya,

On Fri, Dec 21, 2018 at 11:36:48PM +0530, Taniya Das wrote:
> Add support to read the voltage look up table and populate OPP for all
> corresponding CPUS.
> 
> Signed-off-by: Taniya Das 
> ---
>  drivers/cpufreq/qcom-cpufreq-hw.c | 32 ++--
>  1 file changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c 
> b/drivers/cpufreq/qcom-cpufreq-hw.c
> index d83939a..7559b87 100644
> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -10,18 +10,21 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
> 
>  #define LUT_MAX_ENTRIES  40U
>  #define LUT_SRC  GENMASK(31, 30)
>  #define LUT_L_VALGENMASK(7, 0)
>  #define LUT_CORE_COUNT   GENMASK(18, 16)
> +#define LUT_VOLT GENMASK(11, 0)
>  #define LUT_ROW_SIZE 32
>  #define CLK_HW_DIV   2
> 
>  /* Register offsets */
>  #define REG_ENABLE   0x0
> -#define REG_LUT_TABLE0x110
> +#define REG_FREQ_LUT_TABLE   0x110
> +#define REG_VOLT_LUT_TABLE   0x114

The new names suggest that there is a LUT for frequencies and another
one for voltages. I don't have access to hardware documentation, but
from the code and offsets in this driver it seems there is a single
table at offset 0x110, with a 'row' of 32 bytes per OPP. Within this
row the frequency (and other values) is located at offset 0, the
voltage at offset 4.

I'd suggest to keep REG_LUT_TABLE, add a define LUT_OFFSET_VOLTAGE/MV
(or similar) and adjust the math in qcom_cpufreq_hw_read_lut() to use
REG_LUT_TABLE as base offset.

>  #define REG_PERF_STATE   0x920
> 
>  static unsigned long cpu_hw_rate, xo_rate;
> @@ -75,19 +78,26 @@ static int qcom_cpufreq_hw_read_lut(struct device *dev,
>   void __iomem *base)
>  {
>   u32 data, src, lval, i, core_count, prev_cc = 0, prev_freq = 0, freq;
> + u32 volt;
>   unsigned int max_cores = cpumask_weight(policy->cpus);
>   struct cpufreq_frequency_table  *table;
> + unsigned long cpu_r;

nit: why 'cpu_r' and not just 'cpu'?

(if it is needed at all, see my comment below)

> 
>   table = kcalloc(LUT_MAX_ENTRIES + 1, sizeof(*table), GFP_KERNEL);
>   if (!table)
>   return -ENOMEM;
> 
>   for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> - data = readl_relaxed(base + REG_LUT_TABLE + i * LUT_ROW_SIZE);
> + data = readl_relaxed(base + REG_FREQ_LUT_TABLE +
> +   i * LUT_ROW_SIZE);
>   src = FIELD_GET(LUT_SRC, data);
>   lval = FIELD_GET(LUT_L_VAL, data);
>   core_count = FIELD_GET(LUT_CORE_COUNT, data);
> 
> + data = readl_relaxed(base + REG_VOLT_LUT_TABLE +
> +   i * LUT_ROW_SIZE);
> + volt = FIELD_GET(LUT_VOLT, data) * 1000;
> +
>   if (src)
>   freq = xo_rate * lval / 1000;
>   else
> @@ -123,6 +133,10 @@ static int qcom_cpufreq_hw_read_lut(struct device *dev,
> 
>   prev_cc = core_count;
>   prev_freq = freq;
> +
> + freq *= 1000;
> + for_each_cpu(cpu_r, policy->cpus)
> + dev_pm_opp_add(get_cpu_device(cpu_r), freq, volt);

Are you sure we want to duplicate the OPP entries for all CPUs in the
cluster? IIUC the frequencies of the cores in a cluster can't be
changed individually, hence the cores should have a shared table. I
think dev_pm_opp_get_sharing_cpus() does what you need.

You currently also add OPPs for invalid frequencies. From my SDM845
device:

cat /sys/devices/system/cpu/cpufreq/policy4/scaling_available_freq
  => 825600 902400 979200 1056000 1209600 1286400 1363200 1459200
  1536000 1612800 1689600 1766400 1843200 192 1996800 2092800
  2169600 2246400 2323200 240 2476800 2553600 2649600

cat /sys/devices/system/cpu/cpufreq/policy4/scaling_boost_frequencies
2803200

ls /sys/kernel/debug/opp/cpu4/
opp:105600  opp:161280  opp:209280  opp:255360  opp:82560
opp:120960  opp:168960  opp:216960  opp:264960  opp:90240
opp:128640  opp:176640  opp:224640  opp:270720  opp:97920
opp:136320  opp:184320  opp:232320  opp:276480
opp:145920  opp:192000  opp:24  opp:278400
opp:153600  opp:199680  opp:247680  opp:280320

There are OPP entries for 270720, 276480 and 278400 Hz,
however these frequencies appear neither in available_frequencies nor
boost_frequencies.

>   }
> 
>   table[i].frequency = CPUFREQ_TABLE_END;
> @@ -159,10 +173,18 @@ static int qcom_cpufreq_hw_cpu_init(struct 
> cpufreq_policy *policy)
>   struct device *dev = _pdev->dev;
>   struct of_phandle_args args;
>   struct

[PATCH v1] cpufreq: qcom: Read voltage LUT and populate OPP

2018-12-21 Thread Taniya Das

Add support to read the voltage look up table and populate OPP for all
corresponding CPUS.

Signed-off-by: Taniya Das 
---
 drivers/cpufreq/qcom-cpufreq-hw.c | 32 ++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c 
b/drivers/cpufreq/qcom-cpufreq-hw.c
index d83939a..7559b87 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -10,18 +10,21 @@
 #include 
 #include 
 #include 
+#include 
 #include 

 #define LUT_MAX_ENTRIES40U
 #define LUT_SRCGENMASK(31, 30)
 #define LUT_L_VAL  GENMASK(7, 0)
 #define LUT_CORE_COUNT GENMASK(18, 16)
+#define LUT_VOLT   GENMASK(11, 0)
 #define LUT_ROW_SIZE   32
 #define CLK_HW_DIV 2

 /* Register offsets */
 #define REG_ENABLE 0x0
-#define REG_LUT_TABLE  0x110
+#define REG_FREQ_LUT_TABLE 0x110
+#define REG_VOLT_LUT_TABLE 0x114
 #define REG_PERF_STATE 0x920

 static unsigned long cpu_hw_rate, xo_rate;
@@ -75,19 +78,26 @@ static int qcom_cpufreq_hw_read_lut(struct device *dev,
void __iomem *base)
 {
u32 data, src, lval, i, core_count, prev_cc = 0, prev_freq = 0, freq;
+   u32 volt;
unsigned int max_cores = cpumask_weight(policy->cpus);
struct cpufreq_frequency_table  *table;
+   unsigned long cpu_r;

table = kcalloc(LUT_MAX_ENTRIES + 1, sizeof(*table), GFP_KERNEL);
if (!table)
return -ENOMEM;

for (i = 0; i < LUT_MAX_ENTRIES; i++) {
-   data = readl_relaxed(base + REG_LUT_TABLE + i * LUT_ROW_SIZE);
+   data = readl_relaxed(base + REG_FREQ_LUT_TABLE +
+ i * LUT_ROW_SIZE);
src = FIELD_GET(LUT_SRC, data);
lval = FIELD_GET(LUT_L_VAL, data);
core_count = FIELD_GET(LUT_CORE_COUNT, data);

+   data = readl_relaxed(base + REG_VOLT_LUT_TABLE +
+ i * LUT_ROW_SIZE);
+   volt = FIELD_GET(LUT_VOLT, data) * 1000;
+
if (src)
freq = xo_rate * lval / 1000;
else
@@ -123,6 +133,10 @@ static int qcom_cpufreq_hw_read_lut(struct device *dev,

prev_cc = core_count;
prev_freq = freq;
+
+   freq *= 1000;
+   for_each_cpu(cpu_r, policy->cpus)
+   dev_pm_opp_add(get_cpu_device(cpu_r), freq, volt);
}

table[i].frequency = CPUFREQ_TABLE_END;
@@ -159,10 +173,18 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy 
*policy)
struct device *dev = _pdev->dev;
struct of_phandle_args args;
struct device_node *cpu_np;
+   struct device *cpu_dev;
struct resource *res;
void __iomem *base;
int ret, index;

+   cpu_dev = get_cpu_device(policy->cpu);
+   if (!cpu_dev) {
+   pr_err("%s: failed to get cpu%d device\n", __func__,
+  policy->cpu);
+   return -ENODEV;
+   }
+
cpu_np = of_cpu_device_node_get(policy->cpu);
if (!cpu_np)
return -EINVAL;
@@ -205,6 +227,12 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy 
*policy)
goto error;
}

+   ret = dev_pm_opp_get_opp_count(cpu_dev);
+   if (ret <= 0) {
+   dev_err(cpu_dev, "OPP table is not ready\n");
+   goto error;
+   }
+
policy->fast_switch_possible = true;

return 0;
--
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc.is a member
of the Code Aurora Forum, hosted by the  Linux Foundation.

Re: [PATCH v1] cpufreq: qcom: Read voltage LUT and populate OPP

Re: [PATCH v1] cpufreq: qcom: Read voltage LUT and populate OPP

Re: [PATCH v1] cpufreq: qcom: Read voltage LUT and populate OPP

Re: [PATCH v1] cpufreq: qcom: Read voltage LUT and populate OPP

Re: [PATCH v1] cpufreq: qcom: Read voltage LUT and populate OPP

Re: [PATCH v1] cpufreq: qcom: Read voltage LUT and populate OPP

Re: [PATCH v1] cpufreq: qcom: Read voltage LUT and populate OPP

Re: [PATCH v1] cpufreq: qcom: Read voltage LUT and populate OPP

[PATCH v1] cpufreq: qcom: Read voltage LUT and populate OPP

9 matches

Site Navigation

Mail list logo

Footer information