Hi Tomasz,

On Wed, Jul 16, 2014 at 4:33 PM, Tomasz Figa [off-list ref] wrote:

Hi Abhilash, Andrew,

Please see my comments inline.

On 15.07.2014 20:35, Abhilash Kesavan wrote:

quoted

From: Andrew Bresticker <redacted>

Use the common-clock framework to scale frequencies for the various
peripheral clocks on the Exynos5250.

Signed-off-by: Andrew Bresticker <redacted>
Signed-off-by: Abhilash Kesavan <redacted>
---
 drivers/devfreq/exynos/exynos5_bus.c |  131 ++++++++++++++++++++++++++++++----
 1 file changed, 119 insertions(+), 12 deletions(-)

[snip]

quoted

+
 static struct int_bus_opp_table exynos5_int_opp_table[] = {
      {LV_0, 266000, 1025000},
      {LV_1, 200000, 1025000},

@@ -75,6 +85,98 @@ static struct int_bus_opp_table exynos5_int_opp_table[] = {
      {0, 0, 0},
 };

+static struct int_clk_table aclk_166[] = {
+     {LV_0, 167000},
+     {LV_1, 111000},
+     {LV_2,  84000},
+     {LV_3,  84000},
+     {LV_4,  42000},
+};

[snip]

quoted

+static struct int_clk_table aclk_300_gscl[] = {
+     {LV_0, 267000},
+     {LV_1, 267000},
+     {LV_2, 267000},
+     {LV_3, 200000},
+     {LV_4, 100000},
+};

Shouldn't you manage this using OPP framework and parse this from DT?

OK, one of the questions I had was about the handling of virtual INT
bus levels (frequencies and voltages). I have consolidated my
understanding of how the bindings should look and questions I had in
the driver thread.

quoted

+
+#define EXYNOS5_INT_CLK(name, tbl) {         \
+     .clk_name = name,                       \
+     .freq_table = tbl,                      \
+}

[snip]

quoted

@@ -320,16 +427,16 @@ static int exynos5_busfreq_int_probe(struct platform_device *pdev)
      rcu_read_unlock();
      data->curr_freq = initial_freq;

-     err = clk_set_rate(data->int_clk, initial_freq * 1000);
+     err = exynos5_int_setvolt(data, initial_volt);
+     if (err)
+             return err;
+
+     err = exynos5_int_set_rate(data, initial_freq);
      if (err) {
              dev_err(dev, "Failed to set initial frequency\n");
              return err;
      }

-     err = exynos5_int_setvolt(data, initial_volt);
-     if (err)
-             return err;
-

Whether voltage or rate should be set first depends on current settings
and initial_{freq,volt}. Also it might be more convenient to simply call
exynos5_busfreq_int_target() here.

Wouldn't setting voltage first always be safe ? Yes, just calling
target would be better. Will modify.

Regards,
Abhilash

Best regards,
Tomasz