Re: [PATCH v3 02/11] clk: tegra: retrieve regulator info from framework

[Jon Hunter]

On 06/02/18 16:34, Peter De Schrijver wrote:
> The CVB table contains calibration data for the CPU DFLL based on
> process charaterization. The regulator step and offset parameters depend
> on the regulator supplying vdd-cpu , not on the specific Tegra SKU.
> Hence than hardcoding those regulator parameters in the CVB table,
> retrieve them from the regulator framework and store them as part of the
> tegra_dfll_soc_data struct.
> 
> Signed-off-by: Peter De Schrijver 
> ---
>  drivers/clk/tegra/clk-dfll.h   |  2 ++
>  drivers/clk/tegra/clk-tegra124-dfll-fcpu.c | 42 
> +-
>  drivers/clk/tegra/cvb.c| 16 +---
>  drivers/clk/tegra/cvb.h|  6 ++---
>  4 files changed, 53 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/clk/tegra/clk-dfll.h b/drivers/clk/tegra/clk-dfll.h
> index 83352c8..e7cbc5b 100644
> --- a/drivers/clk/tegra/clk-dfll.h
> +++ b/drivers/clk/tegra/clk-dfll.h
> @@ -21,6 +21,7 @@
>  #include 
>  #include 
>  #include 
> +#include "cvb.h"
>  
>  /**
>   * struct tegra_dfll_soc_data - SoC-specific hooks/integration for the DFLL 
> driver
> @@ -35,6 +36,7 @@ struct tegra_dfll_soc_data {
>   struct device *dev;
>   unsigned long max_freq;
>   const struct cvb_table *cvb;
> + struct rail_alignment alignment;
>  
>   void (*init_clock_trimmers)(void);
>   void (*set_clock_trimmers_high)(void);
> diff --git a/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c 
> b/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
> index 269d359..e2dbb79 100644
> --- a/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
> +++ b/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
> @@ -22,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  #include "clk.h"
> @@ -42,9 +43,6 @@
>   .process_id = -1,
>   .min_millivolts = 900,
>   .max_millivolts = 1260,
> - .alignment = {
> - .step_uv = 1, /* 10mV */
> - },
>   .speedo_scale = 100,
>   .voltage_scale = 1000,
>   .entries = {
> @@ -82,6 +80,34 @@
>   },
>  };
>  
> +static int get_alignment_from_regulator(struct device *dev,
> + struct rail_alignment *align)
> +{
> + int min_uV, max_uV, n_voltages, ret;
> + struct regulator *reg;
> +
> + reg = devm_regulator_get(dev, "vdd-cpu");
> + if (IS_ERR(reg))
> + return PTR_ERR(reg);
> +
> + ret = regulator_get_constraint_voltages(reg, _uV, _uV);
> + if (!ret)
> + align->offset_uv = min_uV;
> + else
> + return ret;

Nit-pick ... looks a bit odd, why not ...

if (ret)
return ret;

align->offset_uv = min_uV;

> +
> + align->step_uv = regulator_get_linear_step(reg);
> + if (!align->step_uv && !ret) {

Do you need to test 'ret' here?

> + n_voltages = regulator_count_voltages(reg);
> + if (n_voltages > 1)
> + align->step_uv = (max_uV - min_uV) / (n_voltages - 1);

Later in the patch !align->step_uv is treated as an error, so if
n_voltages should always be greater 1 then why not return an error here?
Seems that this should not happen?

Cheers
Jon

-- 
nvpublic

Re: [PATCH v3 02/11] clk: tegra: retrieve regulator info from framework

[Jon Hunter]

On 06/02/18 16:34, Peter De Schrijver wrote:
> The CVB table contains calibration data for the CPU DFLL based on
> process charaterization. The regulator step and offset parameters depend
> on the regulator supplying vdd-cpu , not on the specific Tegra SKU.
> Hence than hardcoding those regulator parameters in the CVB table,
> retrieve them from the regulator framework and store them as part of the
> tegra_dfll_soc_data struct.
> 
> Signed-off-by: Peter De Schrijver 
> ---
>  drivers/clk/tegra/clk-dfll.h   |  2 ++
>  drivers/clk/tegra/clk-tegra124-dfll-fcpu.c | 42 
> +-
>  drivers/clk/tegra/cvb.c| 16 +---
>  drivers/clk/tegra/cvb.h|  6 ++---
>  4 files changed, 53 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/clk/tegra/clk-dfll.h b/drivers/clk/tegra/clk-dfll.h
> index 83352c8..e7cbc5b 100644
> --- a/drivers/clk/tegra/clk-dfll.h
> +++ b/drivers/clk/tegra/clk-dfll.h
> @@ -21,6 +21,7 @@
>  #include 
>  #include 
>  #include 
> +#include "cvb.h"
>  
>  /**
>   * struct tegra_dfll_soc_data - SoC-specific hooks/integration for the DFLL 
> driver
> @@ -35,6 +36,7 @@ struct tegra_dfll_soc_data {
>   struct device *dev;
>   unsigned long max_freq;
>   const struct cvb_table *cvb;
> + struct rail_alignment alignment;
>  
>   void (*init_clock_trimmers)(void);
>   void (*set_clock_trimmers_high)(void);
> diff --git a/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c 
> b/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
> index 269d359..e2dbb79 100644
> --- a/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
> +++ b/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
> @@ -22,6 +22,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  #include "clk.h"
> @@ -42,9 +43,6 @@
>   .process_id = -1,
>   .min_millivolts = 900,
>   .max_millivolts = 1260,
> - .alignment = {
> - .step_uv = 1, /* 10mV */
> - },
>   .speedo_scale = 100,
>   .voltage_scale = 1000,
>   .entries = {
> @@ -82,6 +80,34 @@
>   },
>  };
>  
> +static int get_alignment_from_regulator(struct device *dev,
> + struct rail_alignment *align)
> +{
> + int min_uV, max_uV, n_voltages, ret;
> + struct regulator *reg;
> +
> + reg = devm_regulator_get(dev, "vdd-cpu");
> + if (IS_ERR(reg))
> + return PTR_ERR(reg);
> +
> + ret = regulator_get_constraint_voltages(reg, _uV, _uV);
> + if (!ret)
> + align->offset_uv = min_uV;
> + else
> + return ret;

Nit-pick ... looks a bit odd, why not ...

if (ret)
return ret;

align->offset_uv = min_uV;

> +
> + align->step_uv = regulator_get_linear_step(reg);
> + if (!align->step_uv && !ret) {

Do you need to test 'ret' here?

> + n_voltages = regulator_count_voltages(reg);
> + if (n_voltages > 1)
> + align->step_uv = (max_uV - min_uV) / (n_voltages - 1);

Later in the patch !align->step_uv is treated as an error, so if
n_voltages should always be greater 1 then why not return an error here?
Seems that this should not happen?

Cheers
Jon

-- 
nvpublic

[PATCH v3 02/11] clk: tegra: retrieve regulator info from framework

[Peter De Schrijver]

The CVB table contains calibration data for the CPU DFLL based on
process charaterization. The regulator step and offset parameters depend
on the regulator supplying vdd-cpu , not on the specific Tegra SKU.
Hence than hardcoding those regulator parameters in the CVB table,
retrieve them from the regulator framework and store them as part of the
tegra_dfll_soc_data struct.

Signed-off-by: Peter De Schrijver 
---
 drivers/clk/tegra/clk-dfll.h   |  2 ++
 drivers/clk/tegra/clk-tegra124-dfll-fcpu.c | 42 +-
 drivers/clk/tegra/cvb.c| 16 +---
 drivers/clk/tegra/cvb.h|  6 ++---
 4 files changed, 53 insertions(+), 13 deletions(-)

diff --git a/drivers/clk/tegra/clk-dfll.h b/drivers/clk/tegra/clk-dfll.h
index 83352c8..e7cbc5b 100644
--- a/drivers/clk/tegra/clk-dfll.h
+++ b/drivers/clk/tegra/clk-dfll.h
@@ -21,6 +21,7 @@
 #include 
 #include 
 #include 
+#include "cvb.h"
 
 /**
  * struct tegra_dfll_soc_data - SoC-specific hooks/integration for the DFLL 
driver
@@ -35,6 +36,7 @@ struct tegra_dfll_soc_data {
struct device *dev;
unsigned long max_freq;
const struct cvb_table *cvb;
+   struct rail_alignment alignment;
 
void (*init_clock_trimmers)(void);
void (*set_clock_trimmers_high)(void);
diff --git a/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c 
b/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
index 269d359..e2dbb79 100644
--- a/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
+++ b/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
@@ -22,6 +22,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include "clk.h"
@@ -42,9 +43,6 @@
.process_id = -1,
.min_millivolts = 900,
.max_millivolts = 1260,
-   .alignment = {
-   .step_uv = 1, /* 10mV */
-   },
.speedo_scale = 100,
.voltage_scale = 1000,
.entries = {
@@ -82,6 +80,34 @@
},
 };
 
+static int get_alignment_from_regulator(struct device *dev,
+   struct rail_alignment *align)
+{
+   int min_uV, max_uV, n_voltages, ret;
+   struct regulator *reg;
+
+   reg = devm_regulator_get(dev, "vdd-cpu");
+   if (IS_ERR(reg))
+   return PTR_ERR(reg);
+
+   ret = regulator_get_constraint_voltages(reg, _uV, _uV);
+   if (!ret)
+   align->offset_uv = min_uV;
+   else
+   return ret;
+
+   align->step_uv = regulator_get_linear_step(reg);
+   if (!align->step_uv && !ret) {
+   n_voltages = regulator_count_voltages(reg);
+   if (n_voltages > 1)
+   align->step_uv = (max_uV - min_uV) / (n_voltages - 1);
+   }
+
+   devm_regulator_put(reg);
+
+   return 0;
+}
+
 static int tegra124_dfll_fcpu_probe(struct platform_device *pdev)
 {
int process_id, speedo_id, speedo_value, err;
@@ -108,10 +134,14 @@ static int tegra124_dfll_fcpu_probe(struct 
platform_device *pdev)
}
 
soc->max_freq = cpu_max_freq_table[speedo_id];
+   err = get_alignment_from_regulator(>dev, >alignment);
+   if (err < 0)
+   return err;
 
-   soc->cvb = tegra_cvb_add_opp_table(soc->dev, tegra124_cpu_cvb_tables,
-  ARRAY_SIZE(tegra124_cpu_cvb_tables),
-  process_id, speedo_id, speedo_value,
+   soc->cvb = tegra_cvb_add_opp_table(soc->dev, fcpu_data->cpu_cvb_tables,
+  fcpu_data->cpu_cvb_tables_size,
+  >alignment, process_id,
+  speedo_id, speedo_value,
   soc->max_freq);
if (IS_ERR(soc->cvb)) {
dev_err(>dev, "couldn't add OPP table: %ld\n",
diff --git a/drivers/clk/tegra/cvb.c b/drivers/clk/tegra/cvb.c
index da9e8e7..2aa9639 100644
--- a/drivers/clk/tegra/cvb.c
+++ b/drivers/clk/tegra/cvb.c
@@ -62,11 +62,17 @@ static int round_voltage(int mv, const struct 
rail_alignment *align, int up)
 }
 
 static int build_opp_table(struct device *dev, const struct cvb_table *table,
+  struct rail_alignment *align,
   int speedo_value, unsigned long max_freq)
 {
-   const struct rail_alignment *align = >alignment;
int i, ret, dfll_mv, min_mv, max_mv;
 
+   if (!align->step_uv)
+   return -EINVAL;
+
+   if (!align->offset_uv)
+   return -EINVAL;
+
min_mv = round_voltage(table->min_millivolts, align, UP);
max_mv = round_voltage(table->max_millivolts, align, DOWN);
 
@@ -109,8 +115,9 @@ static int build_opp_table(struct device *dev, const struct 
cvb_table *table,
  */
 const struct cvb_table *
 tegra_cvb_add_opp_table(struct device *dev, const struct cvb_table 

[PATCH v3 02/11] clk: tegra: retrieve regulator info from framework

[Peter De Schrijver]

The CVB table contains calibration data for the CPU DFLL based on
process charaterization. The regulator step and offset parameters depend
on the regulator supplying vdd-cpu , not on the specific Tegra SKU.
Hence than hardcoding those regulator parameters in the CVB table,
retrieve them from the regulator framework and store them as part of the
tegra_dfll_soc_data struct.

Signed-off-by: Peter De Schrijver 
---
 drivers/clk/tegra/clk-dfll.h   |  2 ++
 drivers/clk/tegra/clk-tegra124-dfll-fcpu.c | 42 +-
 drivers/clk/tegra/cvb.c| 16 +---
 drivers/clk/tegra/cvb.h|  6 ++---
 4 files changed, 53 insertions(+), 13 deletions(-)

diff --git a/drivers/clk/tegra/clk-dfll.h b/drivers/clk/tegra/clk-dfll.h
index 83352c8..e7cbc5b 100644
--- a/drivers/clk/tegra/clk-dfll.h
+++ b/drivers/clk/tegra/clk-dfll.h
@@ -21,6 +21,7 @@
 #include 
 #include 
 #include 
+#include "cvb.h"
 
 /**
  * struct tegra_dfll_soc_data - SoC-specific hooks/integration for the DFLL 
driver
@@ -35,6 +36,7 @@ struct tegra_dfll_soc_data {
struct device *dev;
unsigned long max_freq;
const struct cvb_table *cvb;
+   struct rail_alignment alignment;
 
void (*init_clock_trimmers)(void);
void (*set_clock_trimmers_high)(void);
diff --git a/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c 
b/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
index 269d359..e2dbb79 100644
--- a/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
+++ b/drivers/clk/tegra/clk-tegra124-dfll-fcpu.c
@@ -22,6 +22,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include "clk.h"
@@ -42,9 +43,6 @@
.process_id = -1,
.min_millivolts = 900,
.max_millivolts = 1260,
-   .alignment = {
-   .step_uv = 1, /* 10mV */
-   },
.speedo_scale = 100,
.voltage_scale = 1000,
.entries = {
@@ -82,6 +80,34 @@
},
 };
 
+static int get_alignment_from_regulator(struct device *dev,
+   struct rail_alignment *align)
+{
+   int min_uV, max_uV, n_voltages, ret;
+   struct regulator *reg;
+
+   reg = devm_regulator_get(dev, "vdd-cpu");
+   if (IS_ERR(reg))
+   return PTR_ERR(reg);
+
+   ret = regulator_get_constraint_voltages(reg, _uV, _uV);
+   if (!ret)
+   align->offset_uv = min_uV;
+   else
+   return ret;
+
+   align->step_uv = regulator_get_linear_step(reg);
+   if (!align->step_uv && !ret) {
+   n_voltages = regulator_count_voltages(reg);
+   if (n_voltages > 1)
+   align->step_uv = (max_uV - min_uV) / (n_voltages - 1);
+   }
+
+   devm_regulator_put(reg);
+
+   return 0;
+}
+
 static int tegra124_dfll_fcpu_probe(struct platform_device *pdev)
 {
int process_id, speedo_id, speedo_value, err;
@@ -108,10 +134,14 @@ static int tegra124_dfll_fcpu_probe(struct 
platform_device *pdev)
}
 
soc->max_freq = cpu_max_freq_table[speedo_id];
+   err = get_alignment_from_regulator(>dev, >alignment);
+   if (err < 0)
+   return err;
 
-   soc->cvb = tegra_cvb_add_opp_table(soc->dev, tegra124_cpu_cvb_tables,
-  ARRAY_SIZE(tegra124_cpu_cvb_tables),
-  process_id, speedo_id, speedo_value,
+   soc->cvb = tegra_cvb_add_opp_table(soc->dev, fcpu_data->cpu_cvb_tables,
+  fcpu_data->cpu_cvb_tables_size,
+  >alignment, process_id,
+  speedo_id, speedo_value,
   soc->max_freq);
if (IS_ERR(soc->cvb)) {
dev_err(>dev, "couldn't add OPP table: %ld\n",
diff --git a/drivers/clk/tegra/cvb.c b/drivers/clk/tegra/cvb.c
index da9e8e7..2aa9639 100644
--- a/drivers/clk/tegra/cvb.c
+++ b/drivers/clk/tegra/cvb.c
@@ -62,11 +62,17 @@ static int round_voltage(int mv, const struct 
rail_alignment *align, int up)
 }
 
 static int build_opp_table(struct device *dev, const struct cvb_table *table,
+  struct rail_alignment *align,
   int speedo_value, unsigned long max_freq)
 {
-   const struct rail_alignment *align = >alignment;
int i, ret, dfll_mv, min_mv, max_mv;
 
+   if (!align->step_uv)
+   return -EINVAL;
+
+   if (!align->offset_uv)
+   return -EINVAL;
+
min_mv = round_voltage(table->min_millivolts, align, UP);
max_mv = round_voltage(table->max_millivolts, align, DOWN);
 
@@ -109,8 +115,9 @@ static int build_opp_table(struct device *dev, const struct 
cvb_table *table,
  */
 const struct cvb_table *
 tegra_cvb_add_opp_table(struct device *dev, const struct cvb_table *tables,
-