Devfreq support for Exynos5250/5420

From: Andrew Bresticker <[email protected]>

Devfreq does not support DT-based lookup of these peripheral clocks, so add aliases for them.

Hi Abhilash, Andrew,

Quoted 4 lines · Abhilash Kesavan

From: Andrew Bresticker <[email protected]>

Devfreq does not support DT-based lookup of these peripheral clocks, so add aliases for them.

I'm afraid I have to NAK this. This is going backwards. Please fix the devfreq driver instead.

Best regards,
Tomasz

From: "Arjun.K.V" <[email protected]>

Devfreq does not support DT-based lookup of these peripheral clocks, so add aliases for them.

Signed-off-by Arun Kumar K <[email protected]>

Signed-off-by Arjun.K.V <[email protected]>

Signed-off-by Andrew Bresticker <[email protected]>

Signed-off-by Abhilash Kesavan <[email protected]>

---
 drivers/clk/samsung/clk-exynos5420.c |   73 ++++++++++++++++++++--------------
 1 file changed, 44 insertions(+), 29 deletions(-)

PPMU is required by the exynos5250 devfreq driver. Add a device tree node for it.

Signed-off-by Abhilash Kesavan <[email protected]>

---
 .../bindings/arm/exynos/ppmu-exynos5.txt           |   26 ++++++++++++++++++++
 arch/arm/boot/dts/exynos5250.dtsi                  |    8 ++++++
 2 files changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/exynos/ppmu-exynos5.txt

Documentation/devicetree/bindings/arm/exynos/ppmu-exynos5.txt new file +2 -0

new file mode 100644
index 0000000..a6a2eba
@@ -0,0 +1,26 @@
+Exynos PPMU driver
+-------------------

Hi Abhilash,

Please see my comments inline. Also CCing some DT maintainers.

Quoted 18 lines · Abhilash Kesavan

PPMU is required by the exynos5250 devfreq driver. Add a device tree node for it.

Signed-off-by Abhilash Kesavan <[email protected]>

---
 .../bindings/arm/exynos/ppmu-exynos5.txt           |   26 ++++++++++++++++++++
 arch/arm/boot/dts/exynos5250.dtsi                  |    8 ++++++
 2 files changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/exynos/ppmu-exynos5.txt

Documentation/devicetree/bindings/arm/exynos/ppmu-exynos5.txt new file +2 -0

new file mode 100644
index 0000000..a6a2eba
@@ -0,0 +1,26 @@
+Exynos PPMU driver
+-------------------

Documentation/devicetree/bindings/arm/exynos/ppmu-exynos5.txt line 12

Quoted 10 lines

Documentation/devicetree/bindings/arm/exynos/ppmu-exynos5.txt new file +10 -0

+
+Performance events are primitive values used to get performance data. These
+events provide information about the behavior of the SoC that can be used
+when analyzing system performance. These events are made visible using the
+PPMU logic.
+Exynos PPMU driver is used by the exynos5 devfreq drivers to control the
+bus frequency/voltage.
+
+Required properties:
+- compatible: "samsung,exynos5250-int-busfreq", "samsung,exynos5420-int-busfreq"

If this is a binding for PPMU, shouldn't the compatible string contain the "ppmu" string too? E.g. "samsung,exynos5250-ppmu", "samsung,exynos5420-ppmu".

Documentation/devicetree/bindings/arm/exynos/ppmu-exynos5.txt line 15

Quoted 3 lines

Documentation/devicetree/bindings/arm/exynos/ppmu-exynos5.txt new file +3 -0

+- reg:
+     * physical base address of the PPMUs (e.g DDR, Right Bus, Left bus etc)
+     and length of memory mapped region.

Since PPMU are separate IP blocks, they should have their own device nodes with only address ranges of their own. Representing all PPMUs in the SoC with a single node is incorrect.

Best regards,
Tomasz

arch/arm/boot/dts/exynos5250.dtsi +0 -0

index 834fb5a..5284f00 100644
@@ -778,4 +778,12 @@
 		clocks = <&clock CLK_SSS>;
 		clock-names = "secss";

Actually one more comment.

On 15.07.2014 20:34, Abhilash Kesavan wrote:

[snip]

Quoted 13 lines

arch/arm/boot/dts/exynos5250.dtsi +5 -0

index 834fb5a..5284f00 100644
@@ -778,4 +778,12 @@
              clocks = <&clock CLK_SSS>;
              clock-names = "secss";
      };
+
+     ppmu@10C40000 {
+             compatible = "samsung,exynos5250-int-busfreq";
+             reg = <0x10C40000 0x2000        /* PPMU_DDR_C */
+                    0x10C50000 0x2000        /* PPMU_DDR_L */

arch/arm/boot/dts/exynos5250.dtsi line 789

Quoted 5 lines

arch/arm/boot/dts/exynos5250.dtsi +3 -0

+                    0x10CB0000 0x2000        /* PPMU_DDR_R */
+                    0x13660000 0x2000>;      /* PPMU_RIGHT */
+     };
 };

Please separate patches adding the binding from patches modifying device tree sources. Those are two logically separate changes.

Best regards,
Tomasz

PPMU is required by the exynos5420 devfreq driver. Add a device tree node for it.

Signed-off-by Abhilash Kesavan <[email protected]>

---
  arch/arm/boot/dts/exynos5420.dtsi |    7 +++++++
  1 file changed, 7 insertions(+)

arch/arm/boot/dts/exynos5420.dtsi +0 -0

index 8e7e35c..6ab7b03 100644

Quoted 8 lines · Abhilash Kesavan

PPMU is required by the exynos5420 devfreq driver. Add a device tree node for it.

Signed-off-by Abhilash Kesavan <[email protected]>

---
  arch/arm/boot/dts/exynos5420.dtsi |    7 +++++++
  1 file changed, 7 insertions(+)

arch/arm/boot/dts/exynos5420.dtsi line 837

Quoted 11 lines

arch/arm/boot/dts/exynos5420.dtsi +2 -0

index 8e7e35c..6ab7b03 100644
@@ -833,4 +833,11 @@
                samsung,pmu-syscon = <&pmu_system_controller>;
                #phy-cells = <1>;
        };
+
+       ppmu {

Shouldn't it have the unit-address part, like "ppmu at 10d00000"?

arch/arm/boot/dts/exynos5420.dtsi line 843

Quoted 8 lines

arch/arm/boot/dts/exynos5420.dtsi +5 -0

+               compatible = "samsung,exynos5420-int-busfreq";
+               reg = <0x10D00000 0x2000        /* PPMU_DMC_0_0 */
+                      0x10D10000 0x2000        /* PPMU_DMC_0_1 */
+                      0x10D60000 0x2000>;      /* PPMU_DMC_1_0 */
+       };
  };

WBR, Sergei

From: Andrew Bresticker <[email protected]>

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

Signed-off-by Andrew Bresticker <[email protected]>

Signed-off-by Abhilash Kesavan <[email protected]>

---
 drivers/devfreq/exynos/exynos5_bus.c |  131 ++++++++++++++++++++++++++++++----
 1 file changed, 119 insertions(+), 12 deletions(-)

drivers/devfreq/exynos/exynos5_bus.c +8 -0

+
 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},
+};

drivers/devfreq/exynos/exynos5_bus.c +5 -0

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

drivers/devfreq/exynos/exynos5_bus.c +4 -1

@@ -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;
+

Hi Abhilash, Andrew,

Please see my comments inline.

Quoted 10 lines · Abhilash Kesavan

From: Andrew Bresticker <[email protected]>

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

Signed-off-by Andrew Bresticker <[email protected]>

Signed-off-by Abhilash Kesavan <[email protected]>

---
 drivers/devfreq/exynos/exynos5_bus.c |  131 ++++++++++++++++++++++++++++++----
 1 file changed, 119 insertions(+), 12 deletions(-)

[snip]

Quoted 15 lines

drivers/devfreq/exynos/exynos5_bus.c +8 -0

+
 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]

drivers/devfreq/exynos/exynos5_bus.c static struct int_bus_opp_table exynos5_int_opp_table[] = { · line 134

Quoted 7 lines

drivers/devfreq/exynos/exynos5_bus.c +7 -0

+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?

Quoted 6 lines

drivers/devfreq/exynos/exynos5_bus.c +5 -0

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

[snip]

Quoted 15 lines

drivers/devfreq/exynos/exynos5_bus.c +5 -1

@@ -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;
      }
 

drivers/devfreq/exynos/exynos5_bus.c static int exynos5_busfreq_int_probe(struct platform_device *pdev) · line 332

Quoted 4 lines

drivers/devfreq/exynos/exynos5_bus.c +0 -4

-     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.

Best regards,
Tomasz

From: "Arjun.K.V" <[email protected]>

Exynos5420 bus device devfreq driver monitors PPMU counters and adjusts INT domain operating frequencies and voltages. Support for 5420 has been added to the extant 5250 support.

Signed-off-by Arjun.K.V <[email protected]>

Signed-off-by Andrew Bresticker <[email protected]>

Signed-off-by Doug Anderson <[email protected]>

Signed-off-by Abhilash Kesavan <[email protected]>

---
 drivers/devfreq/Kconfig              |   10 +-
 drivers/devfreq/exynos/exynos5_bus.c |  598 ++++++++++++++++++++++++++++------
 2 files changed, 508 insertions(+), 100 deletions(-)

enum int_level_idx {

(quoted diff) +0 -0

 	LV_0,

@@ -41,12 +46,31 @@ enum int_level_idx {
 	_LV_END
 };

drivers/devfreq/exynos/exynos5_bus.c +3 -0

+
+static struct int_bus_opp_table *exynos5_int_opp_table;
+static struct int_bus_opp_table exynos5250_int_opp_table[] = {
 	{LV_0, 266000, 1025000},
 	{LV_1, 200000, 1025000},
 	{LV_2, 160000, 1025000},

drivers/devfreq/exynos/exynos5_bus.c +22 -0

+
+#define EXYNOS5420_INT_PM_CLK(NAME, CLK, PCLK, CLK_INFO)	\
+static struct int_comp_clks int_pm_clks_##NAME = {	\
+	.mux_clk_name = CLK,				\
+	.div_clk_name = PCLK,				\
+	.clk_info = CLK_INFO,				\
+}
+
+EXYNOS5420_INT_PM_CLK(aclk_200_fsys, "aclk200_fsys",
+			"aclk200_fsys_d", exynos5420_aclk_200_fsys);
+EXYNOS5420_INT_PM_CLK(pclk_200_fsys, "pclk200_fsys",
+			"pclk200_fsys_d", exynos5420_pclk_200_fsys);
+EXYNOS5420_INT_PM_CLK(aclk_100_noc, "aclk100_noc",
+			"aclk100_noc_d", exynos5420_aclk_100_noc);
+EXYNOS5420_INT_PM_CLK(aclk_400_wcore, "aclk400_wcore",
+			"aclk400_wcore_d", exynos5420_aclk_400_wcore);
+EXYNOS5420_INT_PM_CLK(aclk_200_fsys2, "aclk200_fsys2",
+			"aclk200_fsys2_d", exynos5420_aclk_200_fsys2);
+EXYNOS5420_INT_PM_CLK(aclk_400_mscl, "aclk400_mscl",
+			"aclk400_mscl_d", exynos5420_aclk_400_mscl);
+EXYNOS5420_INT_PM_CLK(aclk_166, "aclk166",
+			"aclk166_d", exynos5420_aclk_166);

drivers/devfreq/exynos/exynos5_bus.c +33 -4

+
+static struct int_comp_clks *exynos5420_int_pm_clks[] = {
+	&int_pm_clks_aclk_200_fsys,
+	&int_pm_clks_pclk_200_fsys,
+	&int_pm_clks_aclk_100_noc,
+	&int_pm_clks_aclk_400_wcore,
+	&int_pm_clks_aclk_200_fsys2,
+	&int_pm_clks_aclk_400_mscl,
+	&int_pm_clks_aclk_166,
+	&int_pm_clks_aclk_266,
+	&int_pm_clks_aclk_66,
+	&int_pm_clks_aclk_300_disp1,
+	&int_pm_clks_aclk_300_jpeg,
+	&int_pm_clks_aclk_400_disp1,
+	NULL,
+};
+
+static int exynos5250_int_set_rate(struct busfreq_data_int *data,
 				unsigned long rate)
 {
 	int index, i;
 
-	for (index = 0; index < ARRAY_SIZE(exynos5_int_opp_table); index++) {
-		if (exynos5_int_opp_table[index].clk == rate)
+	for (index = 0; index < ARRAY_SIZE(exynos5250_int_opp_table); index++) {
+		if (exynos5250_int_opp_table[index].clk == rate)
 			break;
 	}

@@ -161,8 +370,8 @@ static int exynos5_int_set_rate(struct busfreq_data_int *data,
 		return -EINVAL;
 
 	/* Change the system clock divider values */
-	for (i = 0; i < ARRAY_SIZE(exynos5_int_clks); i++) {
-		struct int_clk *clk_info = &exynos5_int_clks[i];
+	for (i = 0; i < ARRAY_SIZE(exynos5250_int_clks); i++) {
+		struct int_simple_clk *clk_info = &exynos5250_int_clks[i];
 		int ret;
 
 		ret = clk_set_rate(clk_info->clk,

@@ -177,10 +386,111 @@ static int exynos5_int_set_rate(struct busfreq_data_int *data,
 	return 0;
 }
 
+static struct clk *exynos5420_find_pll(struct busfreq_data_int *data,
+				    enum int_bus_pll target_pll)
+{
+	struct clk *target_src_clk = NULL;
+
+	switch (target_pll) {
+	case C_PLL:
+		target_src_clk = data->mout_cpll;
+		break;
+	case M_PLL:
+		target_src_clk = data->mout_mpll;

drivers/devfreq/exynos/exynos5_bus.c +68 -0

+
+	return target_src_clk;
+}
+
+static int exynos5420_int_set_rate(struct busfreq_data_int *data,
+		unsigned long target_freq, unsigned long pre_freq)
+{
+	unsigned int i;
+	unsigned long tar_rate;
+	int target_idx = -EINVAL;
+	int pre_idx = -EINVAL;
+	struct int_comp_clks *int_clk;
+	struct clk *new_src_pll;
+	struct clk *old_src_pll;
+	unsigned long old_src_rate, new_src_rate;
+	unsigned long rate1, rate2, rate3, rate4;
+
+	/* Find the levels for target and previous frequencies */
+	for (i = 0; i < _LV_END; i++) {
+		if (exynos5420_int_opp_table[i].clk == target_freq)
+			target_idx = exynos5420_int_opp_table[i].idx;
+		if (exynos5420_int_opp_table[i].clk == pre_freq)
+			pre_idx = exynos5420_int_opp_table[i].idx;
+	}
+
+	list_for_each_entry(int_clk, &data->list, node) {
+		tar_rate = int_clk->clk_info[target_idx].target_freq * 1000;
+
+		old_src_pll = clk_get_parent(int_clk->mux_clk);
+		new_src_pll = exynos5420_find_pll(data,
+				int_clk->clk_info[target_idx].src_pll);
+
+		if (old_src_pll == new_src_pll) {
+			/* No need to change pll */
+			clk_set_rate(int_clk->div_clk, tar_rate);
+			pr_debug("%s: %s now %lu (%lu)\n", __func__,
+				 int_clk->mux_clk_name,
+				 clk_get_rate(int_clk->div_clk), tar_rate);
+			continue;
+		}
+
+		old_src_rate = clk_get_rate(old_src_pll);
+		new_src_rate = clk_get_rate(new_src_pll);
+		rate1 = clk_get_rate(int_clk->div_clk);
+
+		/*
+		 * If we're switching to a faster PLL we should bump up the
+		 * divider before switching.
+		 */
+		if (new_src_rate > old_src_rate) {
+			int new_div;
+			unsigned long tmp_rate;
+
+			new_div = DIV_ROUND_UP(new_src_rate, tar_rate);
+			tmp_rate = DIV_ROUND_UP(old_src_rate, new_div);
+			clk_set_rate(int_clk->div_clk, tmp_rate);
+		}
+		rate2 = clk_get_rate(int_clk->div_clk);
+
+		/* We can safely change the mux now */
+		clk_set_parent(int_clk->mux_clk, new_src_pll);
+		rate3 = clk_get_rate(int_clk->div_clk);
+
+		/*
+		 * Give us a proper divider; technically not needed in the case
+		 * where we pre-calculated the divider above (the new_src_rate >
+		 * old_src_rate case), but let's be formal about it.
+		 */

drivers/devfreq/exynos/exynos5_bus.c +7 -8

+
 static int exynos5_int_setvolt(struct busfreq_data_int *data,
 				unsigned long volt)
 {
-	return regulator_set_voltage(data->vdd_int, volt, MAX_SAFEVOLT);
+	return regulator_set_voltage(data->vdd_int, volt,
+				volt + INT_VOLT_STEP_UV);
 }
 
 static int exynos5_busfreq_int_target(struct device *dev, unsigned long *_freq,

@@ -218,18 +528,15 @@ static int exynos5_busfreq_int_target(struct device *dev, unsigned long *_freq,
 	if (data->disabled)
 		goto out;
 
-	if (freq > exynos5_int_opp_table[0].clk)
-		pm_qos_update_request(&data->int_req, freq * 16 / 1000);
-	else
-		pm_qos_update_request(&data->int_req, -1);
-
 	if (old_freq < freq)
 		err = exynos5_int_setvolt(data, volt);
 	if (err)
 		goto out;
 
-	err = exynos5_int_set_rate(data, freq);
-
+	if (data->type == TYPE_BUSF_EXYNOS5250)
+		err = exynos5250_int_set_rate(data, freq);
+	else
+		err = exynos5420_int_set_rate(data, freq, old_freq);
 	if (err)
 		goto out;
 

drivers/devfreq/exynos/exynos5_bus.c +14 -2

 	.target			= exynos5_busfreq_int_target,
 	.get_dev_status		= exynos5_int_get_dev_status,
 };
 
-static int exynos5250_init_int_tables(struct busfreq_data_int *data)
+static int exynos5_init_int_tables(struct busfreq_data_int *data)
 {
 	int i, err = 0;
 
+	if (data->type == TYPE_BUSF_EXYNOS5250)
+		exynos5_int_opp_table = exynos5250_int_opp_table;
+	else
+		exynos5_int_opp_table = exynos5420_int_opp_table;
+
 	for (i = LV_0; i < _LV_END; i++) {
 		err = dev_pm_opp_add(data->dev, exynos5_int_opp_table[i].clk,
 				exynos5_int_opp_table[i].volt);

@@ -297,6 +608,7 @@ static int exynos5_busfreq_int_pm_notifier_event(struct notifier_block *this,
 	struct dev_pm_opp *opp;
 	unsigned long maxfreq = ULONG_MAX;
 	unsigned long freq;
+	unsigned long old_freq;
 	unsigned long volt;
 	int err = 0;
 

@@ -322,8 +634,14 @@ static int exynos5_busfreq_int_pm_notifier_event(struct notifier_block *this,
 		if (err)
 			goto unlock;
 
-		err = exynos5_int_set_rate(data, freq);
+		old_freq = data->curr_freq;
 
+		if (data->type == TYPE_BUSF_EXYNOS5250)
+			err = exynos5250_int_set_rate(data, freq);
+		else if (data->type == TYPE_BUSF_EXYNOS5420)
+			err = exynos5420_int_set_rate(data, freq, old_freq);
+		else
+			err = -EINVAL;
 		if (err)
 			goto unlock;
 

drivers/devfreq/exynos/exynos5_bus.c +32 -4

+	struct exynos5_busfreq_drv_data *data;
+
+	if (pdev->dev.of_node) {
+		const struct of_device_id *match;
+
+		match = of_match_node(exynos5_busfreq_dt_match,
+					pdev->dev.of_node);
+		data = (struct exynos5_busfreq_drv_data *) match->data;
+		return data->busf_type;
+	}
+#endif
+	return platform_get_device_id(pdev)->driver_data;
+}
+
 static int exynos5_busfreq_int_probe(struct platform_device *pdev)
 {
 	struct busfreq_data_int *data;
 	struct busfreq_ppmu_data *ppmu_data;
+	struct device_node *np = pdev->dev.of_node;
 	struct dev_pm_opp *opp;
 	struct device *dev = &pdev->dev;
-	struct device_node *np;
 	unsigned long initial_freq;
 	unsigned long initial_volt;
+	struct clk *mux_clk, *div_clk;
+	struct int_comp_clks *int_pm_clk;
 	int err = 0;
+	int nr_clk;
 	int i;
 
 	data = devm_kzalloc(&pdev->dev, sizeof(struct busfreq_data_int),

@@ -364,22 +704,27 @@ static int exynos5_busfreq_int_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
+	INIT_LIST_HEAD(&data->list);
+	data->type = exynos5_busfreq_get_driver_data(pdev);
+
 	ppmu_data = &data->ppmu_data;
-	ppmu_data->ppmu_end = PPMU_END;
+	if (data->type == TYPE_BUSF_EXYNOS5250) {
+		ppmu_data->ppmu_end = PPMU_END_5250;
+	} else if (data->type == TYPE_BUSF_EXYNOS5420) {
+		ppmu_data->ppmu_end = PPMU_END_5420;
+	} else {
+		dev_err(dev, "Cannot determine the device id %d\n", data->type);
+		return -EINVAL;
+	}
+
 	ppmu_data->ppmu = devm_kzalloc(dev,
-				       sizeof(struct exynos_ppmu) * PPMU_END,
-				       GFP_KERNEL);
+			sizeof(struct exynos_ppmu) * (ppmu_data->ppmu_end),
+			GFP_KERNEL);

drivers/devfreq/exynos/exynos5_bus.c +50 -10

-
 	for (i = 0; i < ppmu_data->ppmu_end; i++) {
 		/* map PPMU memory region */
 		ppmu_data->ppmu[i].hw_base = of_iomap(np, i);

@@ -388,13 +733,17 @@ static int exynos5_busfreq_int_probe(struct platform_device *pdev)
 			return -ENOMEM;
 		}
 	}
+
 	data->pm_notifier.notifier_call = exynos5_busfreq_int_pm_notifier_event;
 	data->dev = dev;
 	mutex_init(&data->lock);
 
-	err = exynos5250_init_int_tables(data);
-	if (err)
+	err = exynos5_init_int_tables(data);
+	if (err) {
+		dev_err(dev, "Cannot initialize busfreq table %d\n",
+			     data->type);
 		return err;
+	}
 
 	data->vdd_int = devm_regulator_get(dev, "vdd_int");
 	if (IS_ERR(data->vdd_int)) {

@@ -402,18 +751,70 @@ static int exynos5_busfreq_int_probe(struct platform_device *pdev)
 		return PTR_ERR(data->vdd_int);
 	}
 
-	for (i = 0; i < ARRAY_SIZE(exynos5_int_clks); i++) {
-		struct int_clk *clk_info = &exynos5_int_clks[i];
+	if (data->type == TYPE_BUSF_EXYNOS5250) {
+		for (i = 0; i < ARRAY_SIZE(exynos5250_int_clks); i++) {
+			struct int_simple_clk *clk_info =
+				&exynos5250_int_clks[i];
+
+			clk_info->clk = devm_clk_get(dev, clk_info->clk_name);
+			if (IS_ERR(clk_info->clk)) {
+				dev_err(dev, "Failed to get clock %s\n",
+					clk_info->clk_name);
+				return PTR_ERR(clk_info->clk);
+			}
+		}
+	} else {
+		data->mout_ipll = devm_clk_get(dev, "mout_ipll");
+		if (IS_ERR(data->mout_ipll)) {
+			dev_err(dev, "Cannot get clock \"mout_ipll\"\n");
+			return PTR_ERR(data->mout_ipll);
+		}
 
-		clk_info->clk = devm_clk_get(dev, clk_info->clk_name);
-		if (IS_ERR(clk_info->clk)) {
-			dev_err(dev, "Failed to get clock %s\n",
-				clk_info->clk_name);
-			return PTR_ERR(clk_info->clk);
+		data->mout_mpll = devm_clk_get(dev, "mout_mpll");
+		if (IS_ERR(data->mout_mpll)) {
+			dev_err(dev, "Cannot get clock \"mout_mpll\"\n");
+			return PTR_ERR(data->mout_mpll);
+		}
+
+		data->mout_dpll = devm_clk_get(dev, "mout_dpll");
+		if (IS_ERR(data->mout_dpll)) {
+			dev_err(dev, "Cannot get clock \"mout_dpll\"\n");
+			return PTR_ERR(data->mout_dpll);
+		}
+
+		data->mout_cpll = devm_clk_get(dev, "mout_cpll");
+		if (IS_ERR(data->mout_cpll)) {
+			dev_err(dev, "Cannot get clock \"mout_cpll\"\n");
+			return PTR_ERR(data->mout_cpll);
+		}
+
+		for (nr_clk = 0; exynos5420_int_pm_clks[nr_clk] != NULL;
+								nr_clk++) {
+			int_pm_clk = exynos5420_int_pm_clks[nr_clk];
+			mux_clk = devm_clk_get(dev, int_pm_clk->mux_clk_name);
+			if (IS_ERR(mux_clk)) {
+				dev_err(dev, "Cannot get mux clock: %s\n",
+						int_pm_clk->mux_clk_name);
+				return PTR_ERR(mux_clk);

Hi Abhilash,

Please see my comments inline.

Quoted 14 lines · Abhilash Kesavan

From: "Arjun.K.V" <[email protected]>

Exynos5420 bus device devfreq driver monitors PPMU counters and adjusts INT domain operating frequencies and voltages. Support for 5420 has been added to the extant 5250 support.

Signed-off-by Arjun.K.V <[email protected]>

Signed-off-by Andrew Bresticker <[email protected]>

Signed-off-by Doug Anderson <[email protected]>

Signed-off-by Abhilash Kesavan <[email protected]>

---
 drivers/devfreq/Kconfig              |   10 +-
 drivers/devfreq/exynos/exynos5_bus.c |  598 ++++++++++++++++++++++++++++------
 2 files changed, 508 insertions(+), 100 deletions(-)

[snip]

Quoted 1 line

drivers/devfreq/exynos/exynos5_bus.c line 28

Quoted 5 lines

drivers/devfreq/exynos/exynos5_bus.c +2 -3

-#define MAX_SAFEVOLT                 1100000 /* 1.10V */
-/* Assume that the bus is saturated if the utilization is 25% */
-#define INT_BUS_SATURATION_RATIO     25
+/* Assume that we need to bump up the level if the utilization is 10% */
+#define INT_BUS_SATURATION_RATIO     10

There is nothing about this change in commit message and changing the ratio from 25% to 10% seems to be rather significant. Please give the rationale for this change.

drivers/devfreq/exynos/exynos5_bus.c line 38

Quoted 11 lines

drivers/devfreq/exynos/exynos5_bus.c +10 -0

+#define INT_VOLT_STEP_UV             12500
+
+struct exynos5_busfreq_drv_data {
+     int busf_type;
+};
+
+enum exynos5_busf_type {
+     TYPE_BUSF_EXYNOS5250,
+     TYPE_BUSF_EXYNOS5420,
+};

Using this kind of enums is discouraged. Rather than that, just create a structure that describes the differences between variants and use this as type.

Quoted 7 lines

 enum int_level_idx {
      LV_0,

(quoted diff) +0 -0

@@ -41,12 +46,31 @@ enum int_level_idx {
      _LV_END
 };

[snip]

Quoted 6 lines

drivers/devfreq/exynos/exynos5_bus.c +3 -0

+
+static struct int_bus_opp_table *exynos5_int_opp_table;
+static struct int_bus_opp_table exynos5250_int_opp_table[] = {
      {LV_0, 266000, 1025000},
      {LV_1, 200000, 1025000},
      {LV_2, 160000, 1025000},

[snip]

drivers/devfreq/exynos/exynos5_bus.c static struct int_clk_table aclk_300_gscl[] = { · line 309

Quoted 8 lines

drivers/devfreq/exynos/exynos5_bus.c +8 -0

+static struct int_clk_info exynos5420_aclk_300_jpeg[] = {
+     /* Level, Freq, Parent_Pll */
+     {LV_0, 300000, D_PLL},
+     {LV_1, 300000, D_PLL},
+     {LV_2, 200000, D_PLL},
+     {LV_3, 150000, D_PLL},
+     {LV_4,  75000, D_PLL},
+};

These should be parsed from DT.

Quoted 23 lines

drivers/devfreq/exynos/exynos5_bus.c +22 -0

+
+#define EXYNOS5420_INT_PM_CLK(NAME, CLK, PCLK, CLK_INFO)     \
+static struct int_comp_clks int_pm_clks_##NAME = {   \
+     .mux_clk_name = CLK,                            \
+     .div_clk_name = PCLK,                           \
+     .clk_info = CLK_INFO,                           \
+}
+
+EXYNOS5420_INT_PM_CLK(aclk_200_fsys, "aclk200_fsys",
+                     "aclk200_fsys_d", exynos5420_aclk_200_fsys);
+EXYNOS5420_INT_PM_CLK(pclk_200_fsys, "pclk200_fsys",
+                     "pclk200_fsys_d", exynos5420_pclk_200_fsys);
+EXYNOS5420_INT_PM_CLK(aclk_100_noc, "aclk100_noc",
+                     "aclk100_noc_d", exynos5420_aclk_100_noc);
+EXYNOS5420_INT_PM_CLK(aclk_400_wcore, "aclk400_wcore",
+                     "aclk400_wcore_d", exynos5420_aclk_400_wcore);
+EXYNOS5420_INT_PM_CLK(aclk_200_fsys2, "aclk200_fsys2",
+                     "aclk200_fsys2_d", exynos5420_aclk_200_fsys2);
+EXYNOS5420_INT_PM_CLK(aclk_400_mscl, "aclk400_mscl",
+                     "aclk400_mscl_d", exynos5420_aclk_400_mscl);
+EXYNOS5420_INT_PM_CLK(aclk_166, "aclk166",
+                     "aclk166_d", exynos5420_aclk_166);

drivers/devfreq/exynos/exynos5_bus.c static struct int_clk_table aclk_300_gscl[] = { · line 341

Quoted 10 lines

drivers/devfreq/exynos/exynos5_bus.c +10 -0

+EXYNOS5420_INT_PM_CLK(aclk_266, "aclk266",
+                     "aclk266_d", exynos5420_aclk_266);
+EXYNOS5420_INT_PM_CLK(aclk_66, "aclk66",
+                     "aclk66_d", exynos5420_aclk_66);
+EXYNOS5420_INT_PM_CLK(aclk_300_disp1, "aclk300_disp1",
+                     "aclk300_disp1_d", exynos5420_aclk_300_disp1);
+EXYNOS5420_INT_PM_CLK(aclk_300_jpeg, "aclk300_jpeg",
+                     "aclk300_jpeg_d", exynos5420_aclk_300_jpeg);
+EXYNOS5420_INT_PM_CLK(aclk_400_disp1, "aclk400_disp1",
+                     "aclk400_disp1_d", exynos5420_aclk_400_disp1);

List of the clocks should be parsed from DT as well, without hardcoding data for every SoC in the driver.

Quoted 60 lines

drivers/devfreq/exynos/exynos5_bus.c +38 -4

+
+static struct int_comp_clks *exynos5420_int_pm_clks[] = {
+     &int_pm_clks_aclk_200_fsys,
+     &int_pm_clks_pclk_200_fsys,
+     &int_pm_clks_aclk_100_noc,
+     &int_pm_clks_aclk_400_wcore,
+     &int_pm_clks_aclk_200_fsys2,
+     &int_pm_clks_aclk_400_mscl,
+     &int_pm_clks_aclk_166,
+     &int_pm_clks_aclk_266,
+     &int_pm_clks_aclk_66,
+     &int_pm_clks_aclk_300_disp1,
+     &int_pm_clks_aclk_300_jpeg,
+     &int_pm_clks_aclk_400_disp1,
+     NULL,
+};
+
+static int exynos5250_int_set_rate(struct busfreq_data_int *data,
                              unsigned long rate)
 {
      int index, i;
 
-     for (index = 0; index < ARRAY_SIZE(exynos5_int_opp_table); index++) {
-             if (exynos5_int_opp_table[index].clk == rate)
+     for (index = 0; index < ARRAY_SIZE(exynos5250_int_opp_table); index++) {
+             if (exynos5250_int_opp_table[index].clk == rate)
                      break;
      }

@@ -161,8 +370,8 @@ static int exynos5_int_set_rate(struct busfreq_data_int *data,
              return -EINVAL;
 
      /* Change the system clock divider values */
-     for (i = 0; i < ARRAY_SIZE(exynos5_int_clks); i++) {
-             struct int_clk *clk_info = &exynos5_int_clks[i];
+     for (i = 0; i < ARRAY_SIZE(exynos5250_int_clks); i++) {
+             struct int_simple_clk *clk_info = &exynos5250_int_clks[i];
              int ret;
 
              ret = clk_set_rate(clk_info->clk,

@@ -177,10 +386,111 @@ static int exynos5_int_set_rate(struct busfreq_data_int *data,
      return 0;
 }
 
+static struct clk *exynos5420_find_pll(struct busfreq_data_int *data,
+                                 enum int_bus_pll target_pll)
+{
+     struct clk *target_src_clk = NULL;
+
+     switch (target_pll) {
+     case C_PLL:
+             target_src_clk = data->mout_cpll;
+             break;
+     case M_PLL:
+             target_src_clk = data->mout_mpll;
+             break;
+     case D_PLL:
+             target_src_clk = data->mout_dpll;
+             break;
+     case I_PLL:

drivers/devfreq/exynos/exynos5_bus.c static int exynos5_int_set_rate(struct busfreq_data_int *data, · line 409

Quoted 5 lines

drivers/devfreq/exynos/exynos5_bus.c +5 -0

+             target_src_clk = data->mout_ipll;
+             break;
+     default:
+             break;
+     }

What about storing the clocks in an array? Then all you would need to do could be as simple as accessing data->plls[target_pll].

Quoted 74 lines

drivers/devfreq/exynos/exynos5_bus.c +73 -0

+
+     return target_src_clk;
+}
+
+static int exynos5420_int_set_rate(struct busfreq_data_int *data,
+             unsigned long target_freq, unsigned long pre_freq)
+{
+     unsigned int i;
+     unsigned long tar_rate;
+     int target_idx = -EINVAL;
+     int pre_idx = -EINVAL;
+     struct int_comp_clks *int_clk;
+     struct clk *new_src_pll;
+     struct clk *old_src_pll;
+     unsigned long old_src_rate, new_src_rate;
+     unsigned long rate1, rate2, rate3, rate4;
+
+     /* Find the levels for target and previous frequencies */
+     for (i = 0; i < _LV_END; i++) {
+             if (exynos5420_int_opp_table[i].clk == target_freq)
+                     target_idx = exynos5420_int_opp_table[i].idx;
+             if (exynos5420_int_opp_table[i].clk == pre_freq)
+                     pre_idx = exynos5420_int_opp_table[i].idx;
+     }
+
+     list_for_each_entry(int_clk, &data->list, node) {
+             tar_rate = int_clk->clk_info[target_idx].target_freq * 1000;
+
+             old_src_pll = clk_get_parent(int_clk->mux_clk);
+             new_src_pll = exynos5420_find_pll(data,
+                             int_clk->clk_info[target_idx].src_pll);
+
+             if (old_src_pll == new_src_pll) {
+                     /* No need to change pll */
+                     clk_set_rate(int_clk->div_clk, tar_rate);
+                     pr_debug("%s: %s now %lu (%lu)\n", __func__,
+                              int_clk->mux_clk_name,
+                              clk_get_rate(int_clk->div_clk), tar_rate);
+                     continue;
+             }
+
+             old_src_rate = clk_get_rate(old_src_pll);
+             new_src_rate = clk_get_rate(new_src_pll);
+             rate1 = clk_get_rate(int_clk->div_clk);
+
+             /*
+              * If we're switching to a faster PLL we should bump up the
+              * divider before switching.
+              */
+             if (new_src_rate > old_src_rate) {
+                     int new_div;
+                     unsigned long tmp_rate;
+
+                     new_div = DIV_ROUND_UP(new_src_rate, tar_rate);
+                     tmp_rate = DIV_ROUND_UP(old_src_rate, new_div);
+                     clk_set_rate(int_clk->div_clk, tmp_rate);
+             }
+             rate2 = clk_get_rate(int_clk->div_clk);
+
+             /* We can safely change the mux now */
+             clk_set_parent(int_clk->mux_clk, new_src_pll);
+             rate3 = clk_get_rate(int_clk->div_clk);
+
+             /*
+              * Give us a proper divider; technically not needed in the case
+              * where we pre-calculated the divider above (the new_src_rate >
+              * old_src_rate case), but let's be formal about it.
+              */
+             clk_set_rate(int_clk->div_clk, tar_rate);
+             rate4 = clk_get_rate(int_clk->div_clk);
+
+             pr_debug("%s: %s => PLL %d; %lu=>%lu=>%lu=>%lu (%lu)\n",
+                      __func__, int_clk->mux_clk_name,

drivers/devfreq/exynos/exynos5_bus.c static int exynos5_int_set_rate(struct busfreq_data_int *data, · line 487

Quoted 5 lines

drivers/devfreq/exynos/exynos5_bus.c +5 -0

+                      int_clk->clk_info[target_idx].src_pll,
+                      rate1, rate2, rate3, rate4, tar_rate);
+     }
+     return 0;
+}

The above function looks like it could be made much more generic and reused for Exynos5250 as well.

Quoted 35 lines

drivers/devfreq/exynos/exynos5_bus.c +7 -8

+
 static int exynos5_int_setvolt(struct busfreq_data_int *data,
                              unsigned long volt)
 {
-     return regulator_set_voltage(data->vdd_int, volt, MAX_SAFEVOLT);
+     return regulator_set_voltage(data->vdd_int, volt,
+                             volt + INT_VOLT_STEP_UV);
 }
 
 static int exynos5_busfreq_int_target(struct device *dev, unsigned long *_freq,

@@ -218,18 +528,15 @@ static int exynos5_busfreq_int_target(struct device *dev, unsigned long *_freq,
      if (data->disabled)
              goto out;
 
-     if (freq > exynos5_int_opp_table[0].clk)
-             pm_qos_update_request(&data->int_req, freq * 16 / 1000);
-     else
-             pm_qos_update_request(&data->int_req, -1);
-
      if (old_freq < freq)
              err = exynos5_int_setvolt(data, volt);
      if (err)
              goto out;
 
-     err = exynos5_int_set_rate(data, freq);
-
+     if (data->type == TYPE_BUSF_EXYNOS5250)
+             err = exynos5250_int_set_rate(data, freq);
+     else
+             err = exynos5420_int_set_rate(data, freq, old_freq);
      if (err)
              goto out;
 

drivers/devfreq/exynos/exynos5_bus.c static int exynos5_int_get_dev_status(struct device *dev, · line 577

Quoted 6 lines

drivers/devfreq/exynos/exynos5_bus.c +1 -2

@@ -267,16 +574,20 @@ static int exynos5_int_get_dev_status(struct device *dev,
 }
 
 static struct devfreq_dev_profile exynos5_devfreq_int_profile = {
-     .initial_freq           = 160000,
-     .polling_ms             = 100,
+     .polling_ms             = 10,

Another change not mentioned in commit message.

Quoted 45 lines

drivers/devfreq/exynos/exynos5_bus.c +14 -2

      .target                 = exynos5_busfreq_int_target,
      .get_dev_status         = exynos5_int_get_dev_status,
 };
 
-static int exynos5250_init_int_tables(struct busfreq_data_int *data)
+static int exynos5_init_int_tables(struct busfreq_data_int *data)
 {
      int i, err = 0;
 
+     if (data->type == TYPE_BUSF_EXYNOS5250)
+             exynos5_int_opp_table = exynos5250_int_opp_table;
+     else
+             exynos5_int_opp_table = exynos5420_int_opp_table;
+
      for (i = LV_0; i < _LV_END; i++) {
              err = dev_pm_opp_add(data->dev, exynos5_int_opp_table[i].clk,
                              exynos5_int_opp_table[i].volt);

@@ -297,6 +608,7 @@ static int exynos5_busfreq_int_pm_notifier_event(struct notifier_block *this,
      struct dev_pm_opp *opp;
      unsigned long maxfreq = ULONG_MAX;
      unsigned long freq;
+     unsigned long old_freq;
      unsigned long volt;
      int err = 0;
 

@@ -322,8 +634,14 @@ static int exynos5_busfreq_int_pm_notifier_event(struct notifier_block *this,
              if (err)
                      goto unlock;
 
-             err = exynos5_int_set_rate(data, freq);
+             old_freq = data->curr_freq;
 
+             if (data->type == TYPE_BUSF_EXYNOS5250)
+                     err = exynos5250_int_set_rate(data, freq);
+             else if (data->type == TYPE_BUSF_EXYNOS5420)
+                     err = exynos5420_int_set_rate(data, freq, old_freq);
+             else
+                     err = -EINVAL;
              if (err)
                      goto unlock;
 

@@ -345,16 +663,38 @@ unlock:
      return NOTIFY_DONE;
 }
 

drivers/devfreq/exynos/exynos5_bus.c unlock: · line 670

Quoted 5 lines

drivers/devfreq/exynos/exynos5_bus.c +5 -0

+static const struct of_device_id exynos5_busfreq_dt_match[];
+
+static inline int exynos5_busfreq_get_driver_data(struct platform_device *pdev)
+{
+#ifdef CONFIG_OF

Exynos is DT-only, so there is no need to handle non-DT cases.

Quoted 55 lines

drivers/devfreq/exynos/exynos5_bus.c +32 -4

+     struct exynos5_busfreq_drv_data *data;
+
+     if (pdev->dev.of_node) {
+             const struct of_device_id *match;
+
+             match = of_match_node(exynos5_busfreq_dt_match,
+                                     pdev->dev.of_node);
+             data = (struct exynos5_busfreq_drv_data *) match->data;
+             return data->busf_type;
+     }
+#endif
+     return platform_get_device_id(pdev)->driver_data;
+}
+
 static int exynos5_busfreq_int_probe(struct platform_device *pdev)
 {
      struct busfreq_data_int *data;
      struct busfreq_ppmu_data *ppmu_data;
+     struct device_node *np = pdev->dev.of_node;
      struct dev_pm_opp *opp;
      struct device *dev = &pdev->dev;
-     struct device_node *np;
      unsigned long initial_freq;
      unsigned long initial_volt;
+     struct clk *mux_clk, *div_clk;
+     struct int_comp_clks *int_pm_clk;
      int err = 0;
+     int nr_clk;
      int i;
 
      data = devm_kzalloc(&pdev->dev, sizeof(struct busfreq_data_int),

@@ -364,22 +704,27 @@ static int exynos5_busfreq_int_probe(struct platform_device *pdev)
              return -ENOMEM;
      }
 
+     INIT_LIST_HEAD(&data->list);
+     data->type = exynos5_busfreq_get_driver_data(pdev);
+
      ppmu_data = &data->ppmu_data;
-     ppmu_data->ppmu_end = PPMU_END;
+     if (data->type == TYPE_BUSF_EXYNOS5250) {
+             ppmu_data->ppmu_end = PPMU_END_5250;
+     } else if (data->type == TYPE_BUSF_EXYNOS5420) {
+             ppmu_data->ppmu_end = PPMU_END_5420;
+     } else {
+             dev_err(dev, "Cannot determine the device id %d\n", data->type);
+             return -EINVAL;
+     }
+
      ppmu_data->ppmu = devm_kzalloc(dev,
-                                    sizeof(struct exynos_ppmu) * PPMU_END,
-                                    GFP_KERNEL);
+                     sizeof(struct exynos_ppmu) * (ppmu_data->ppmu_end),
+                     GFP_KERNEL);

drivers/devfreq/exynos/exynos5_bus.c static int exynos5_busfreq_int_probe(struct platform_device *pdev) · line 381

Quoted 10 lines

drivers/devfreq/exynos/exynos5_bus.c +0 -5

      if (!ppmu_data->ppmu) {
              dev_err(dev, "Failed to allocate memory for exynos_ppmu\n");
              return -ENOMEM;
      }
 
-     np = of_find_compatible_node(NULL, NULL, "samsung,exynos5250-ppmu");
-     if (np == NULL) {
-             pr_err("Unable to find PPMU node\n");
-             return -ENOENT;
-     }

This was actually closer to the right solution than what this series does. Actually there was similar attempt already, but for Exynos4 and I even suggested how this could be handled properly. Please see [1] for the whole discussion.

[1]
https://www.mail-archive.com/[email protected]/msg27491.html

Quoted 81 lines

drivers/devfreq/exynos/exynos5_bus.c +50 -10

-
      for (i = 0; i < ppmu_data->ppmu_end; i++) {
              /* map PPMU memory region */
              ppmu_data->ppmu[i].hw_base = of_iomap(np, i);

@@ -388,13 +733,17 @@ static int exynos5_busfreq_int_probe(struct platform_device *pdev)
                      return -ENOMEM;
              }
      }
+
      data->pm_notifier.notifier_call = exynos5_busfreq_int_pm_notifier_event;
      data->dev = dev;
      mutex_init(&data->lock);
 
-     err = exynos5250_init_int_tables(data);
-     if (err)
+     err = exynos5_init_int_tables(data);
+     if (err) {
+             dev_err(dev, "Cannot initialize busfreq table %d\n",
+                          data->type);
              return err;
+     }
 
      data->vdd_int = devm_regulator_get(dev, "vdd_int");
      if (IS_ERR(data->vdd_int)) {

@@ -402,18 +751,70 @@ static int exynos5_busfreq_int_probe(struct platform_device *pdev)
              return PTR_ERR(data->vdd_int);
      }
 
-     for (i = 0; i < ARRAY_SIZE(exynos5_int_clks); i++) {
-             struct int_clk *clk_info = &exynos5_int_clks[i];
+     if (data->type == TYPE_BUSF_EXYNOS5250) {
+             for (i = 0; i < ARRAY_SIZE(exynos5250_int_clks); i++) {
+                     struct int_simple_clk *clk_info =
+                             &exynos5250_int_clks[i];
+
+                     clk_info->clk = devm_clk_get(dev, clk_info->clk_name);
+                     if (IS_ERR(clk_info->clk)) {
+                             dev_err(dev, "Failed to get clock %s\n",
+                                     clk_info->clk_name);
+                             return PTR_ERR(clk_info->clk);
+                     }
+             }
+     } else {
+             data->mout_ipll = devm_clk_get(dev, "mout_ipll");
+             if (IS_ERR(data->mout_ipll)) {
+                     dev_err(dev, "Cannot get clock \"mout_ipll\"\n");
+                     return PTR_ERR(data->mout_ipll);
+             }
 
-             clk_info->clk = devm_clk_get(dev, clk_info->clk_name);
-             if (IS_ERR(clk_info->clk)) {
-                     dev_err(dev, "Failed to get clock %s\n",
-                             clk_info->clk_name);
-                     return PTR_ERR(clk_info->clk);
+             data->mout_mpll = devm_clk_get(dev, "mout_mpll");
+             if (IS_ERR(data->mout_mpll)) {
+                     dev_err(dev, "Cannot get clock \"mout_mpll\"\n");
+                     return PTR_ERR(data->mout_mpll);
+             }
+
+             data->mout_dpll = devm_clk_get(dev, "mout_dpll");
+             if (IS_ERR(data->mout_dpll)) {
+                     dev_err(dev, "Cannot get clock \"mout_dpll\"\n");
+                     return PTR_ERR(data->mout_dpll);
+             }
+
+             data->mout_cpll = devm_clk_get(dev, "mout_cpll");
+             if (IS_ERR(data->mout_cpll)) {
+                     dev_err(dev, "Cannot get clock \"mout_cpll\"\n");
+                     return PTR_ERR(data->mout_cpll);
+             }
+
+             for (nr_clk = 0; exynos5420_int_pm_clks[nr_clk] != NULL;
+                                                             nr_clk++) {
+                     int_pm_clk = exynos5420_int_pm_clks[nr_clk];
+                     mux_clk = devm_clk_get(dev, int_pm_clk->mux_clk_name);
+                     if (IS_ERR(mux_clk)) {
+                             dev_err(dev, "Cannot get mux clock: %s\n",
+                                             int_pm_clk->mux_clk_name);
+                             return PTR_ERR(mux_clk);

drivers/devfreq/exynos/exynos5_bus.c static int exynos5_busfreq_int_probe(struct platform_device *pdev) · line 808

Quoted 10 lines

drivers/devfreq/exynos/exynos5_bus.c +10 -0

+                     }
+                     div_clk = devm_clk_get(dev, int_pm_clk->div_clk_name);
+                     if (IS_ERR(div_clk)) {
+                             dev_err(dev, "Cannot get div clock: %s\n",
+                                             int_pm_clk->div_clk_name);
+                             return PTR_ERR(div_clk);
+                     }
+                     int_pm_clk->mux_clk = mux_clk;
+                     int_pm_clk->div_clk = div_clk;
+                     list_add_tail(&int_pm_clk->node, &data->list);

All those could be probably handled with an array and a for loop.

In general, this patch apparently contains many separate changes and not only is hard to review but also hard to debug potential problems - git bisect has commit granularity.

Please refactor the driver step by step first and then add support for new SoC after it has all the needed features.

Best regards,
Tomasz

Hi Tomasz,

Quoted 23 lines · Tomasz Figa

Hi Abhilash,

Please see my comments inline.

Quoted 14 lines · Abhilash Kesavan

From: "Arjun.K.V" <[email protected]>

Exynos5420 bus device devfreq driver monitors PPMU counters and adjusts INT domain operating frequencies and voltages. Support for 5420 has been added to the extant 5250 support.

Signed-off-by Arjun.K.V <[email protected]>

Signed-off-by Andrew Bresticker <[email protected]>

Signed-off-by Doug Anderson <[email protected]>

Signed-off-by Abhilash Kesavan <[email protected]>

---
 drivers/devfreq/Kconfig              |   10 +-
 drivers/devfreq/exynos/exynos5_bus.c |  598 ++++++++++++++++++++++++++++------
 2 files changed, 508 insertions(+), 100 deletions(-)

[snip]

Quoted 1 line

Quoted 9 lines

drivers/devfreq/exynos/exynos5_bus.c line 28

Quoted 5 lines

drivers/devfreq/exynos/exynos5_bus.c +2 -3

-#define MAX_SAFEVOLT                 1100000 /* 1.10V */
-/* Assume that the bus is saturated if the utilization is 25% */
-#define INT_BUS_SATURATION_RATIO     25
+/* Assume that we need to bump up the level if the utilization is 10% */
+#define INT_BUS_SATURATION_RATIO     10

There is nothing about this change in commit message and changing the ratio from 25% to 10% seems to be rather significant. Please give the rationale for this change.

Will do.

Quoted 15 lines

drivers/devfreq/exynos/exynos5_bus.c line 38

Quoted 11 lines

drivers/devfreq/exynos/exynos5_bus.c +10 -0

+#define INT_VOLT_STEP_UV             12500
+
+struct exynos5_busfreq_drv_data {
+     int busf_type;
+};
+
+enum exynos5_busf_type {
+     TYPE_BUSF_EXYNOS5250,
+     TYPE_BUSF_EXYNOS5420,
+};

Using this kind of enums is discouraged. Rather than that, just create a structure that describes the differences between variants and use this as type.

OK.

Quoted 19 lines

Quoted 7 lines

 enum int_level_idx {
      LV_0,

(quoted diff) +0 -0

@@ -41,12 +46,31 @@ enum int_level_idx {
      _LV_END
 };

[snip]

Quoted 6 lines

drivers/devfreq/exynos/exynos5_bus.c +3 -0

+
+static struct int_bus_opp_table *exynos5_int_opp_table;
+static struct int_bus_opp_table exynos5250_int_opp_table[] = {
      {LV_0, 266000, 1025000},
      {LV_1, 200000, 1025000},
      {LV_2, 160000, 1025000},

[snip]

Quoted 10 lines

drivers/devfreq/exynos/exynos5_bus.c static struct int_clk_table aclk_300_gscl[] = { · line 309

Quoted 8 lines

drivers/devfreq/exynos/exynos5_bus.c +8 -0

+static struct int_clk_info exynos5420_aclk_300_jpeg[] = {
+     /* Level, Freq, Parent_Pll */
+     {LV_0, 300000, D_PLL},
+     {LV_1, 300000, D_PLL},
+     {LV_2, 200000, D_PLL},
+     {LV_3, 150000, D_PLL},
+     {LV_4,  75000, D_PLL},
+};

These should be parsed from DT.

OK.

Quoted 97 lines

Quoted 23 lines

drivers/devfreq/exynos/exynos5_bus.c +22 -0

+
+#define EXYNOS5420_INT_PM_CLK(NAME, CLK, PCLK, CLK_INFO)     \
+static struct int_comp_clks int_pm_clks_##NAME = {   \
+     .mux_clk_name = CLK,                            \
+     .div_clk_name = PCLK,                           \
+     .clk_info = CLK_INFO,                           \
+}
+
+EXYNOS5420_INT_PM_CLK(aclk_200_fsys, "aclk200_fsys",
+                     "aclk200_fsys_d", exynos5420_aclk_200_fsys);
+EXYNOS5420_INT_PM_CLK(pclk_200_fsys, "pclk200_fsys",
+                     "pclk200_fsys_d", exynos5420_pclk_200_fsys);
+EXYNOS5420_INT_PM_CLK(aclk_100_noc, "aclk100_noc",
+                     "aclk100_noc_d", exynos5420_aclk_100_noc);
+EXYNOS5420_INT_PM_CLK(aclk_400_wcore, "aclk400_wcore",
+                     "aclk400_wcore_d", exynos5420_aclk_400_wcore);
+EXYNOS5420_INT_PM_CLK(aclk_200_fsys2, "aclk200_fsys2",
+                     "aclk200_fsys2_d", exynos5420_aclk_200_fsys2);
+EXYNOS5420_INT_PM_CLK(aclk_400_mscl, "aclk400_mscl",
+                     "aclk400_mscl_d", exynos5420_aclk_400_mscl);
+EXYNOS5420_INT_PM_CLK(aclk_166, "aclk166",
+                     "aclk166_d", exynos5420_aclk_166);

drivers/devfreq/exynos/exynos5_bus.c static struct int_clk_table aclk_300_gscl[] = { · line 341

Quoted 10 lines

drivers/devfreq/exynos/exynos5_bus.c +10 -0

+EXYNOS5420_INT_PM_CLK(aclk_266, "aclk266",
+                     "aclk266_d", exynos5420_aclk_266);
+EXYNOS5420_INT_PM_CLK(aclk_66, "aclk66",
+                     "aclk66_d", exynos5420_aclk_66);
+EXYNOS5420_INT_PM_CLK(aclk_300_disp1, "aclk300_disp1",
+                     "aclk300_disp1_d", exynos5420_aclk_300_disp1);
+EXYNOS5420_INT_PM_CLK(aclk_300_jpeg, "aclk300_jpeg",
+                     "aclk300_jpeg_d", exynos5420_aclk_300_jpeg);
+EXYNOS5420_INT_PM_CLK(aclk_400_disp1, "aclk400_disp1",
+                     "aclk400_disp1_d", exynos5420_aclk_400_disp1);

List of the clocks should be parsed from DT as well, without hardcoding data for every SoC in the driver.

Quoted 60 lines

drivers/devfreq/exynos/exynos5_bus.c +38 -4

+
+static struct int_comp_clks *exynos5420_int_pm_clks[] = {
+     &int_pm_clks_aclk_200_fsys,
+     &int_pm_clks_pclk_200_fsys,
+     &int_pm_clks_aclk_100_noc,
+     &int_pm_clks_aclk_400_wcore,
+     &int_pm_clks_aclk_200_fsys2,
+     &int_pm_clks_aclk_400_mscl,
+     &int_pm_clks_aclk_166,
+     &int_pm_clks_aclk_266,
+     &int_pm_clks_aclk_66,
+     &int_pm_clks_aclk_300_disp1,
+     &int_pm_clks_aclk_300_jpeg,
+     &int_pm_clks_aclk_400_disp1,
+     NULL,
+};
+
+static int exynos5250_int_set_rate(struct busfreq_data_int *data,
                              unsigned long rate)
 {
      int index, i;
 
-     for (index = 0; index < ARRAY_SIZE(exynos5_int_opp_table); index++) {
-             if (exynos5_int_opp_table[index].clk == rate)
+     for (index = 0; index < ARRAY_SIZE(exynos5250_int_opp_table); index++) {
+             if (exynos5250_int_opp_table[index].clk == rate)
                      break;
      }

@@ -161,8 +370,8 @@ static int exynos5_int_set_rate(struct busfreq_data_int *data,
              return -EINVAL;
 
      /* Change the system clock divider values */
-     for (i = 0; i < ARRAY_SIZE(exynos5_int_clks); i++) {
-             struct int_clk *clk_info = &exynos5_int_clks[i];
+     for (i = 0; i < ARRAY_SIZE(exynos5250_int_clks); i++) {
+             struct int_simple_clk *clk_info = &exynos5250_int_clks[i];
              int ret;
 
              ret = clk_set_rate(clk_info->clk,

@@ -177,10 +386,111 @@ static int exynos5_int_set_rate(struct busfreq_data_int *data,
      return 0;
 }
 
+static struct clk *exynos5420_find_pll(struct busfreq_data_int *data,
+                                 enum int_bus_pll target_pll)
+{
+     struct clk *target_src_clk = NULL;
+
+     switch (target_pll) {
+     case C_PLL:
+             target_src_clk = data->mout_cpll;
+             break;
+     case M_PLL:
+             target_src_clk = data->mout_mpll;
+             break;
+     case D_PLL:
+             target_src_clk = data->mout_dpll;
+             break;
+     case I_PLL:

Quoted 8 lines

drivers/devfreq/exynos/exynos5_bus.c static int exynos5_int_set_rate(struct busfreq_data_int *data, · line 409

Quoted 5 lines

drivers/devfreq/exynos/exynos5_bus.c +5 -0

+             target_src_clk = data->mout_ipll;
+             break;
+     default:
+             break;
+     }

What about storing the clocks in an array? Then all you would need to do could be as simple as accessing data->plls[target_pll].

OK.

Quoted 74 lines

Quoted 74 lines

drivers/devfreq/exynos/exynos5_bus.c +73 -0

+
+     return target_src_clk;
+}
+
+static int exynos5420_int_set_rate(struct busfreq_data_int *data,
+             unsigned long target_freq, unsigned long pre_freq)
+{
+     unsigned int i;
+     unsigned long tar_rate;
+     int target_idx = -EINVAL;
+     int pre_idx = -EINVAL;
+     struct int_comp_clks *int_clk;
+     struct clk *new_src_pll;
+     struct clk *old_src_pll;
+     unsigned long old_src_rate, new_src_rate;
+     unsigned long rate1, rate2, rate3, rate4;
+
+     /* Find the levels for target and previous frequencies */
+     for (i = 0; i < _LV_END; i++) {
+             if (exynos5420_int_opp_table[i].clk == target_freq)
+                     target_idx = exynos5420_int_opp_table[i].idx;
+             if (exynos5420_int_opp_table[i].clk == pre_freq)
+                     pre_idx = exynos5420_int_opp_table[i].idx;
+     }
+
+     list_for_each_entry(int_clk, &data->list, node) {
+             tar_rate = int_clk->clk_info[target_idx].target_freq * 1000;
+
+             old_src_pll = clk_get_parent(int_clk->mux_clk);
+             new_src_pll = exynos5420_find_pll(data,
+                             int_clk->clk_info[target_idx].src_pll);
+
+             if (old_src_pll == new_src_pll) {
+                     /* No need to change pll */
+                     clk_set_rate(int_clk->div_clk, tar_rate);
+                     pr_debug("%s: %s now %lu (%lu)\n", __func__,
+                              int_clk->mux_clk_name,
+                              clk_get_rate(int_clk->div_clk), tar_rate);
+                     continue;
+             }
+
+             old_src_rate = clk_get_rate(old_src_pll);
+             new_src_rate = clk_get_rate(new_src_pll);
+             rate1 = clk_get_rate(int_clk->div_clk);
+
+             /*
+              * If we're switching to a faster PLL we should bump up the
+              * divider before switching.
+              */
+             if (new_src_rate > old_src_rate) {
+                     int new_div;
+                     unsigned long tmp_rate;
+
+                     new_div = DIV_ROUND_UP(new_src_rate, tar_rate);
+                     tmp_rate = DIV_ROUND_UP(old_src_rate, new_div);
+                     clk_set_rate(int_clk->div_clk, tmp_rate);
+             }
+             rate2 = clk_get_rate(int_clk->div_clk);
+
+             /* We can safely change the mux now */
+             clk_set_parent(int_clk->mux_clk, new_src_pll);
+             rate3 = clk_get_rate(int_clk->div_clk);
+
+             /*
+              * Give us a proper divider; technically not needed in the case
+              * where we pre-calculated the divider above (the new_src_rate >
+              * old_src_rate case), but let's be formal about it.
+              */
+             clk_set_rate(int_clk->div_clk, tar_rate);
+             rate4 = clk_get_rate(int_clk->div_clk);
+
+             pr_debug("%s: %s => PLL %d; %lu=>%lu=>%lu=>%lu (%lu)\n",
+                      __func__, int_clk->mux_clk_name,

Quoted 8 lines

drivers/devfreq/exynos/exynos5_bus.c static int exynos5_int_set_rate(struct busfreq_data_int *data, · line 487

Quoted 5 lines

drivers/devfreq/exynos/exynos5_bus.c +5 -0

+                      int_clk->clk_info[target_idx].src_pll,
+                      rate1, rate2, rate3, rate4, tar_rate);
+     }
+     return 0;
+}

The above function looks like it could be made much more generic and reused for Exynos5250 as well.

I will look at making it common for the two,

Quoted 89 lines

Quoted 35 lines

drivers/devfreq/exynos/exynos5_bus.c +7 -8

+
 static int exynos5_int_setvolt(struct busfreq_data_int *data,
                              unsigned long volt)
 {
-     return regulator_set_voltage(data->vdd_int, volt, MAX_SAFEVOLT);
+     return regulator_set_voltage(data->vdd_int, volt,
+                             volt + INT_VOLT_STEP_UV);
 }
 
 static int exynos5_busfreq_int_target(struct device *dev, unsigned long *_freq,

@@ -218,18 +528,15 @@ static int exynos5_busfreq_int_target(struct device *dev, unsigned long *_freq,
      if (data->disabled)
              goto out;
 
-     if (freq > exynos5_int_opp_table[0].clk)
-             pm_qos_update_request(&data->int_req, freq * 16 / 1000);
-     else
-             pm_qos_update_request(&data->int_req, -1);
-
      if (old_freq < freq)
              err = exynos5_int_setvolt(data, volt);
      if (err)
              goto out;
 
-     err = exynos5_int_set_rate(data, freq);
-
+     if (data->type == TYPE_BUSF_EXYNOS5250)
+             err = exynos5250_int_set_rate(data, freq);
+     else
+             err = exynos5420_int_set_rate(data, freq, old_freq);
      if (err)
              goto out;
 

drivers/devfreq/exynos/exynos5_bus.c static int exynos5_int_get_dev_status(struct device *dev, · line 577

Quoted 6 lines

drivers/devfreq/exynos/exynos5_bus.c +1 -2

@@ -267,16 +574,20 @@ static int exynos5_int_get_dev_status(struct device *dev,
 }
 
 static struct devfreq_dev_profile exynos5_devfreq_int_profile = {
-     .initial_freq           = 160000,
-     .polling_ms             = 100,
+     .polling_ms             = 10,

Another change not mentioned in commit message.

Quoted 45 lines

drivers/devfreq/exynos/exynos5_bus.c +14 -2

      .target                 = exynos5_busfreq_int_target,
      .get_dev_status         = exynos5_int_get_dev_status,
 };
 
-static int exynos5250_init_int_tables(struct busfreq_data_int *data)
+static int exynos5_init_int_tables(struct busfreq_data_int *data)
 {
      int i, err = 0;
 
+     if (data->type == TYPE_BUSF_EXYNOS5250)
+             exynos5_int_opp_table = exynos5250_int_opp_table;
+     else
+             exynos5_int_opp_table = exynos5420_int_opp_table;
+
      for (i = LV_0; i < _LV_END; i++) {
              err = dev_pm_opp_add(data->dev, exynos5_int_opp_table[i].clk,
                              exynos5_int_opp_table[i].volt);

@@ -297,6 +608,7 @@ static int exynos5_busfreq_int_pm_notifier_event(struct notifier_block *this,
      struct dev_pm_opp *opp;
      unsigned long maxfreq = ULONG_MAX;
      unsigned long freq;
+     unsigned long old_freq;
      unsigned long volt;
      int err = 0;
 

@@ -322,8 +634,14 @@ static int exynos5_busfreq_int_pm_notifier_event(struct notifier_block *this,
              if (err)
                      goto unlock;
 
-             err = exynos5_int_set_rate(data, freq);
+             old_freq = data->curr_freq;
 
+             if (data->type == TYPE_BUSF_EXYNOS5250)
+                     err = exynos5250_int_set_rate(data, freq);
+             else if (data->type == TYPE_BUSF_EXYNOS5420)
+                     err = exynos5420_int_set_rate(data, freq, old_freq);
+             else
+                     err = -EINVAL;
              if (err)
                      goto unlock;
 

@@ -345,16 +663,38 @@ unlock:
      return NOTIFY_DONE;
 }
 

Quoted 7 lines

drivers/devfreq/exynos/exynos5_bus.c unlock: · line 670

Quoted 5 lines

drivers/devfreq/exynos/exynos5_bus.c +5 -0

+static const struct of_device_id exynos5_busfreq_dt_match[];
+
+static inline int exynos5_busfreq_get_driver_data(struct platform_device *pdev)
+{
+#ifdef CONFIG_OF

Exynos is DT-only, so there is no need to handle non-DT cases.

OK.

Quoted 71 lines

Quoted 55 lines

drivers/devfreq/exynos/exynos5_bus.c +32 -4

+     struct exynos5_busfreq_drv_data *data;
+
+     if (pdev->dev.of_node) {
+             const struct of_device_id *match;
+
+             match = of_match_node(exynos5_busfreq_dt_match,
+                                     pdev->dev.of_node);
+             data = (struct exynos5_busfreq_drv_data *) match->data;
+             return data->busf_type;
+     }
+#endif
+     return platform_get_device_id(pdev)->driver_data;
+}
+
 static int exynos5_busfreq_int_probe(struct platform_device *pdev)
 {
      struct busfreq_data_int *data;
      struct busfreq_ppmu_data *ppmu_data;
+     struct device_node *np = pdev->dev.of_node;
      struct dev_pm_opp *opp;
      struct device *dev = &pdev->dev;
-     struct device_node *np;
      unsigned long initial_freq;
      unsigned long initial_volt;
+     struct clk *mux_clk, *div_clk;
+     struct int_comp_clks *int_pm_clk;
      int err = 0;
+     int nr_clk;
      int i;
 
      data = devm_kzalloc(&pdev->dev, sizeof(struct busfreq_data_int),

@@ -364,22 +704,27 @@ static int exynos5_busfreq_int_probe(struct platform_device *pdev)
              return -ENOMEM;
      }
 
+     INIT_LIST_HEAD(&data->list);
+     data->type = exynos5_busfreq_get_driver_data(pdev);
+
      ppmu_data = &data->ppmu_data;
-     ppmu_data->ppmu_end = PPMU_END;
+     if (data->type == TYPE_BUSF_EXYNOS5250) {
+             ppmu_data->ppmu_end = PPMU_END_5250;
+     } else if (data->type == TYPE_BUSF_EXYNOS5420) {
+             ppmu_data->ppmu_end = PPMU_END_5420;
+     } else {
+             dev_err(dev, "Cannot determine the device id %d\n", data->type);
+             return -EINVAL;
+     }
+
      ppmu_data->ppmu = devm_kzalloc(dev,
-                                    sizeof(struct exynos_ppmu) * PPMU_END,
-                                    GFP_KERNEL);
+                     sizeof(struct exynos_ppmu) * (ppmu_data->ppmu_end),
+                     GFP_KERNEL);

drivers/devfreq/exynos/exynos5_bus.c static int exynos5_busfreq_int_probe(struct platform_device *pdev) · line 381

Quoted 10 lines

drivers/devfreq/exynos/exynos5_bus.c +0 -5

      if (!ppmu_data->ppmu) {
              dev_err(dev, "Failed to allocate memory for exynos_ppmu\n");
              return -ENOMEM;
      }
 
-     np = of_find_compatible_node(NULL, NULL, "samsung,exynos5250-ppmu");
-     if (np == NULL) {
-             pr_err("Unable to find PPMU node\n");
-             return -ENOENT;
-     }

This was actually closer to the right solution than what this series does. Actually there was similar attempt already, but for Exynos4 and I even suggested how this could be handled properly. Please see [1] for the whole discussion.

Quoted 2 lines

[1]
https://www.mail-archive.com/[email protected]/msg27491.html

Chanwoo are you working on this for exynos4 ? As far as I can see there has been no further effort in adding the power plane and ppmu bindings. So, before I start working on this I wanted to confirm that you don't have something in the pipeline.

Tomasz, I have read through the thread and as per your suggestions this is how the dt bindings (for my INT use case) would look like:

ppmu_dmc0_0: ppmu@10D00000 {
        /* Resources of PPMU */
}

ppmu_dmc0_1: ppmu@10D00000 {
        /* Resources of PPMU */
}

ppmu_dmc_1_0: ppmu@10D00000 {
        /* Resources of PPMU */
}

power-plane-int {
        compatible = "exynos5420-int, power-plane";
        samsung,ppmus = <&ppmu_dmc_0_0>, &ppmu_dmc_0_1>, &ppmu_dmc_1_0>;
        samsung,devices = ??
        clock-names = "aclk200_fsys", "pclk200_fsys", "aclk100_noc"...;
        clocks = <&clock CLK_ACLK200_FSYS2>,...;
        vdd_int-supply = <&buck3_reg>; /* VDD_INT */
        operating-points = <
                                   266000, 1300000
                                   200000, 1250000
                                   160000, 1225000
                                   133000, 1200000
                                   >;
};

Here are my doubts regarding this:
1) The INT bus consists of several IPs like MFC, DISP, HDMI, ISP etc. The 3 PPMUs listed above monitor the load on the TOP block which these IPs are a part of. What should be the "samsung, devices" associated with it ?
2) I have currently listed the virtual INT bus frequencies in "operating points" property. So, when there is a high load on the INT bus, there would be a request for say a virtual frequency of 266MHz which in turn would bump up several clocks (like MFC, DISP etc) to a