[RFC v2 0/4] Add basic support for ASV

[RFC v2 0/4] Add basic support for ASV

[Sachin Kamat]

Original cover letter from Yadwinder:
This series is to add basic common infrastructure for ASV.
 Basically ASV is a technique used on samsung SoCs, which provides the
recommended supply voltage for dvfs of arm, mif etc. For a given operating
frequency, the voltage is recommended based on SoC's ASV group.
ASV group gets fussed on SoCs during process of mass production.

This series includes:
 - basic common infrastructue for ASV. It provides common APIs for user drivers
like cpufreq & devfreq and and an interface for SoC specific drivers to
register ASV members(instances)
 - a common platform driver to register ASV members for exynos SoCs
 - an example providing minimal support (only for ARM ASV) for exynos5250 chips

Its just basic skelton which I wanted to get it reviewed or discussed in
early stage, before going ahead on further development based on it.
 Presently example is based on static ASV table provided in SoC specific file,
which I expects to go into DT. But exactly how and where needs to be discussed,
may be in next revisions once we get through the basic skelton.
 Also the location of driver in kernel may also seem odd to someone and
many more things :).

Looking for your valuable reviews and suggestions.

Changes since v1:
* Rebased onto the latest linux-next
* Used devm* and *opp APIs
* Code cleanup and some fixes
* Updated kernel doc and Kconfig text

Yadwinder Singh Brar (4):
  power: asv: Add common ASV support for Samsung SoCs
  power: asv: Add a common ASV driver for Exynos SoCs.
  power: asv: Add support for Exynos5250
  ARM: SAMSUNG: Register static platform device for ASV for Exynos5

 arch/arm/mach-exynos/mach-exynos5-dt.c |    2 +
 drivers/power/Kconfig                  |    1 +
 drivers/power/Makefile                 |    1 +
 drivers/power/asv/Kconfig              |   23 +++++
 drivers/power/asv/Makefile             |    2 +
 drivers/power/asv/asv.c                |  176 ++++++++++++++++++++++++++++++++
 drivers/power/asv/exynos-asv.c         |   78 ++++++++++++++
 drivers/power/asv/exynos-asv.h         |   22 ++++
 drivers/power/asv/exynos5250-asv.c     |  139 +++++++++++++++++++++++++
 include/linux/power/asv-driver.h       |   62 +++++++++++
 include/linux/power/asv.h              |   37 +++++++
 11 files changed, 543 insertions(+)
 create mode 100644 drivers/power/asv/Kconfig
 create mode 100644 drivers/power/asv/Makefile
 create mode 100644 drivers/power/asv/asv.c
 create mode 100644 drivers/power/asv/exynos-asv.c
 create mode 100644 drivers/power/asv/exynos-asv.h
 create mode 100644 drivers/power/asv/exynos5250-asv.c
 create mode 100644 include/linux/power/asv-driver.h
 create mode 100644 include/linux/power/asv.h

-- 
1.7.9.5

[RFC v2 1/4] power: asv: Add common ASV support for Samsung SoCs

[Sachin Kamat]

From: Yadwinder Singh Brar <yadi.brar@samsung.com>

This patch introduces a common ASV (Adaptive Supply Voltage) basic framework
for samsung SoCs. It provides common APIs (to be called by users to get ASV
values or init opp_table) and an interface for SoC specific drivers to
register ASV members (instances).

Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
---
 drivers/power/Kconfig            |    1 +
 drivers/power/Makefile           |    1 +
 drivers/power/asv/Kconfig        |   10 +++
 drivers/power/asv/Makefile       |    1 +
 drivers/power/asv/asv.c          |  176 ++++++++++++++++++++++++++++++++++++++
 include/linux/power/asv-driver.h |   62 ++++++++++++++
 include/linux/power/asv.h        |   37 ++++++++
 7 files changed, 288 insertions(+)
 create mode 100644 drivers/power/asv/Kconfig
 create mode 100644 drivers/power/asv/Makefile
 create mode 100644 drivers/power/asv/asv.c
 create mode 100644 include/linux/power/asv-driver.h
 create mode 100644 include/linux/power/asv.h

diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index 5e2054afe840..09da1fd730cd 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -385,3 +385,4 @@ source "drivers/power/reset/Kconfig"
 endif # POWER_SUPPLY
 
 source "drivers/power/avs/Kconfig"
+source "drivers/power/asv/Kconfig"
diff --git a/drivers/power/Makefile b/drivers/power/Makefile
index 372b4e8ab598..788e36d37d24 100644
--- a/drivers/power/Makefile
+++ b/drivers/power/Makefile
@@ -54,6 +54,7 @@ obj-$(CONFIG_CHARGER_BQ2415X)	+= bq2415x_charger.o
 obj-$(CONFIG_CHARGER_BQ24190)	+= bq24190_charger.o
 obj-$(CONFIG_CHARGER_BQ24735)	+= bq24735-charger.o
 obj-$(CONFIG_POWER_AVS)		+= avs/
+obj-$(CONFIG_POWER_ASV)		+= asv/
 obj-$(CONFIG_CHARGER_SMB347)	+= smb347-charger.o
 obj-$(CONFIG_CHARGER_TPS65090)	+= tps65090-charger.o
 obj-$(CONFIG_POWER_RESET)	+= reset/
diff --git a/drivers/power/asv/Kconfig b/drivers/power/asv/Kconfig
new file mode 100644
index 000000000000..761119d9f7f8
--- /dev/null
+++ b/drivers/power/asv/Kconfig
@@ -0,0 +1,10 @@
+menuconfig POWER_ASV
+	bool "Adaptive Supply Voltage (ASV) support"
+	help
+	  ASV is a technique used on Samsung SoCs which provides the
+	  recommended supply voltage for some specific parts(like CPU, MIF, etc)
+	  that support DVFS. For a given operating frequency, the voltage is
+	  recommended based on SoCs ASV group. ASV group info is provided in the
+	  chip id info which depends on the chip manufacturing process.
+
+	  Say Y here to enable Adaptive Supply Voltage support.
diff --git a/drivers/power/asv/Makefile b/drivers/power/asv/Makefile
new file mode 100644
index 000000000000..366cb04f557b
--- /dev/null
+++ b/drivers/power/asv/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_POWER_ASV)			+= asv.o
diff --git a/drivers/power/asv/asv.c b/drivers/power/asv/asv.c
new file mode 100644
index 000000000000..3f2c31a0d3a9
--- /dev/null
+++ b/drivers/power/asv/asv.c
@@ -0,0 +1,176 @@
+/*
+ * ASV(Adaptive Supply Voltage) common core
+ *
+ * Copyright (c) 2013 Samsung Electronics Co., Ltd.
+ *		http://www.samsung.com/
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+*/
+
+#include <linux/io.h>
+#include <linux/pm_opp.h>
+#include <linux/slab.h>
+#include <linux/device.h>
+#include <linux/power/asv-driver.h>
+
+static LIST_HEAD(asv_list);
+static DEFINE_MUTEX(asv_mutex);
+
+struct asv_member {
+	struct list_head		node;
+	struct asv_info *asv_info;
+};
+
+static void add_asv_member(struct asv_member *asv_mem)
+{
+	mutex_lock(&asv_mutex);
+	list_add_tail(&asv_mem->node, &asv_list);
+	mutex_unlock(&asv_mutex);
+}
+
+static struct asv_member *asv_get_mem(enum asv_type_id asv_type)
+{
+	struct asv_member *asv_mem;
+	struct asv_info *asv_info;
+
+	list_for_each_entry(asv_mem, &asv_list, node) {
+		asv_info = asv_mem->asv_info;
+		if (asv_type == asv_info->type)
+			return asv_mem;
+	}
+
+	return NULL;
+}
+
+unsigned int asv_get_volt(enum asv_type_id target_type,
+						unsigned int target_freq)
+{
+	struct asv_member *asv_mem = asv_get_mem(target_type);
+	struct asv_freq_table *dvfs_table;
+	struct asv_info *asv_info;
+	unsigned int i;
+
+	if (!asv_mem)
+		return 0;
+
+	asv_info = asv_mem->asv_info;
+	dvfs_table = asv_info->dvfs_table;
+
+	for (i = 0; i < asv_info->nr_dvfs_level; i++) {
+		if (dvfs_table[i].freq == target_freq)
+			return dvfs_table[i].volt;
+	}
+
+	return 0;
+}
+
+int asv_init_opp_table(struct device *dev, enum asv_type_id target_type)
+{
+	struct asv_member *asv_mem = asv_get_mem(target_type);
+	struct asv_info *asv_info;
+	struct asv_freq_table *dvfs_table;
+	unsigned int i;
+
+	if (!asv_mem)
+		return -EINVAL;
+
+	asv_info = asv_mem->asv_info;
+	dvfs_table = asv_info->dvfs_table;
+
+	for (i = 0; i < asv_info->nr_dvfs_level; i++) {
+		if (dev_pm_opp_add(dev, dvfs_table[i].freq * 1000,
+			dvfs_table[i].volt)) {
+			dev_warn(dev, "Failed to add OPP %d\n",
+				 dvfs_table[i].freq);
+			continue;
+		}
+	}
+
+	return 0;
+}
+
+static struct asv_member *asv_init_member(struct asv_info *asv_info)
+{
+	struct asv_member *asv_mem;
+	int ret = 0;
+
+	if (!asv_info) {
+		pr_err("No ASV info provided\n");
+		return NULL;
+	}
+
+	asv_mem = kzalloc(sizeof(*asv_mem), GFP_KERNEL);
+	if (!asv_mem) {
+		pr_err("Allocation failed for member: %s\n", asv_info->name);
+		return NULL;
+	}
+
+	asv_mem->asv_info = kmemdup(asv_info, sizeof(*asv_info), GFP_KERNEL);
+	if (!asv_mem->asv_info) {
+		pr_err("Copying asv_info failed for member: %s\n",
+			asv_info->name);
+		kfree(asv_mem);
+		return NULL;
+	}
+	asv_info = asv_mem->asv_info;
+
+	if (asv_info->ops->get_asv_group) {
+		ret = asv_info->ops->get_asv_group(asv_info);
+		if (ret) {
+			pr_err("get_asv_group failed for %s : %d\n",
+				asv_info->name, ret);
+			goto err;
+		}
+	}
+
+	if (asv_info->ops->init_asv)
+		ret = asv_info->ops->init_asv(asv_info);
+		if (ret) {
+			pr_err("asv_init failed for %s : %d\n",
+				asv_info->name, ret);
+			goto err;
+		}
+
+	/* In case of parsing table from DT, we may need to add flag to identify
+	DT supporting members and call init_asv_table from asv_init_opp_table(
+	after getting dev_node from dev,if required), instead of calling here.
+	*/
+
+	if (asv_info->ops->init_asv_table) {
+		ret = asv_info->ops->init_asv_table(asv_info);
+		if (ret) {
+			pr_err("init_asv_table failed for %s : %d\n",
+				asv_info->name, ret);
+			goto err;
+		}
+	}
+
+	if (!asv_info->nr_dvfs_level || !asv_info->dvfs_table) {
+		pr_err("No dvfs_table for %s\n", asv_info->name);
+		goto err;
+	}
+
+	pr_info("Registered asv member: %s with group: %d",
+		asv_info->name, asv_info->asv_grp);
+
+	return asv_mem;
+err:
+	kfree(asv_mem->asv_info);
+	kfree(asv_mem);
+	return NULL;
+}
+
+void register_asv_member(struct asv_info *list, unsigned int nr_member)
+{
+	struct asv_member *asv_mem;
+	int cnt;
+
+	for (cnt = 0; cnt < nr_member; cnt++) {
+		asv_mem = asv_init_member(&list[cnt]);
+
+		if (asv_mem)
+			add_asv_member(asv_mem);
+	}
+}
diff --git a/include/linux/power/asv-driver.h b/include/linux/power/asv-driver.h
new file mode 100644
index 000000000000..afe072cbd451
--- /dev/null
+++ b/include/linux/power/asv-driver.h
@@ -0,0 +1,62 @@
+/*
+ * Adaptive Supply Voltage Driver Header File
+ *
+ * copyright (c) 2013 samsung electronics co., ltd.
+ *		http://www.samsung.com/
+ *
+ * this program is free software; you can redistribute it and/or modify
+ * it under the terms of the gnu general public license version 2 as
+ * published by the free software foundation.
+*/
+
+#ifndef __ASV_D_H
+#define __ASV_D_H __FILE__
+
+#include <linux/power/asv.h>
+
+struct asv_freq_table {
+	unsigned int	freq;	/* KHz */
+	unsigned int	volt;	/* uV */
+};
+
+/* struct asv_info - information of ASV member for intialisation
+ *
+ * Each member to be registered should be described using this struct
+ * intialised with all required information for that member.
+ *
+ * @name: Name to use for member.
+ * @asv_type_id: Type to identify particular member.
+ * @asv_ops: Callbacks which can be used for SoC specific operations.
+ * @nr_dvfs_level: Number of dvfs levels supported by member.
+ * @dvfs_table: Table containing supported ASV freqs and corresponding volts.
+ * @asv_grp: ASV group of member.
+ * @flags: ASV flags
+ */
+struct asv_info {
+	const char		*name;
+	enum asv_type_id	type;
+	struct asv_ops		*ops;
+	unsigned int		nr_dvfs_level;
+	struct asv_freq_table	*dvfs_table;
+	unsigned int		asv_grp;
+	unsigned int		flags;
+};
+
+/* struct asv_ops - SoC specific operation for ASV members
+ * @get_asv_group - Calculates and initializes asv_grp of asv_info.
+ * @init_asv - SoC specific initialisation (if required) based on asv_grp.
+ * @init_asv_table - Initializes linear array(dvfs_table) for corresponding
+ *			asv_grp.
+ *
+ * All ops should return 0 on sucess.
+ */
+struct asv_ops {
+	int (*init_asv)(struct asv_info *);
+	int (*get_asv_group)(struct asv_info *);
+	int (*init_asv_table)(struct asv_info *);
+};
+
+/* function for registering ASV members */
+void register_asv_member(struct asv_info *list, unsigned int nr_member);
+
+#endif /* __ASV_D_H */
diff --git a/include/linux/power/asv.h b/include/linux/power/asv.h
new file mode 100644
index 000000000000..bfc4e4fa8719
--- /dev/null
+++ b/include/linux/power/asv.h
@@ -0,0 +1,37 @@
+/*
+ * Adaptive Supply Voltage Header File
+ *
+ * copyright (c) 2013 samsung electronics co., ltd.
+ *		http://www.samsung.com/
+ *
+ * this program is free software; you can redistribute it and/or modify
+ * it under the terms of the gnu general public license version 2 as
+ * published by the free software foundation.
+*/
+
+#ifndef __ASV_H
+#define __ASV_H __FILE__
+
+enum asv_type_id {
+	ASV_ARM,
+	ASV_INT,
+	ASV_MIF,
+	ASV_G3D,
+};
+
+#ifdef CONFIG_POWER_ASV
+/* asv_get_volt - get the ASV for target_freq for particular target_type.
+ *	returns 0 if target_freq is not supported
+ */
+extern unsigned int asv_get_volt(enum asv_type_id target_type,
+					unsigned int target_freq);
+extern int asv_init_opp_table(struct device *dev,
+					enum asv_type_id target_type);
+#else
+static inline unsigned int asv_get_volt(enum asv_type_id target_type,
+				unsigned int target_freq) { return 0; }
+static int asv_init_opp_table(struct device *dev, enum asv_type_id target_type)
+	{ return 0; }
+
+#endif /* CONFIG_POWER_EXYNOS_AVS */
+#endif /* __ASV_H */
-- 
1.7.9.5

[RFC v2 2/4] power: asv: Add a common ASV driver for Exynos SoCs.

[Sachin Kamat]

From: Yadwinder Singh Brar <yadi.brar@samsung.com>

This patch adds a common platform driver to register ASV members for
Exynos SoCs.

Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
---
 drivers/power/asv/Kconfig      |   13 ++++++++
 drivers/power/asv/Makefile     |    1 +
 drivers/power/asv/exynos-asv.c |   70 ++++++++++++++++++++++++++++++++++++++++
 drivers/power/asv/exynos-asv.h |   21 ++++++++++++
 4 files changed, 105 insertions(+)
 create mode 100644 drivers/power/asv/exynos-asv.c
 create mode 100644 drivers/power/asv/exynos-asv.h

diff --git a/drivers/power/asv/Kconfig b/drivers/power/asv/Kconfig
index 761119d9f7f8..a57980a33148 100644
--- a/drivers/power/asv/Kconfig
+++ b/drivers/power/asv/Kconfig
@@ -8,3 +8,16 @@ menuconfig POWER_ASV
 	  chip id info which depends on the chip manufacturing process.
 
 	  Say Y here to enable Adaptive Supply Voltage support.
+
+if POWER_ASV
+
+config POWER_EXYNOS_ASV
+	bool "Adaptive Supply Voltage for Exynos"
+	help
+	  Exynos supports ASV depending upon the ASV group fused on chip.
+	  Users can request ASV specific to a frequency for a particular member
+	  from corresponding DVFS driver.
+
+	  Say Y here to enable Exynos Adaptive Supply Voltage.
+
+endif
diff --git a/drivers/power/asv/Makefile b/drivers/power/asv/Makefile
index 366cb04f557b..9d0e0cc32cb5 100644
--- a/drivers/power/asv/Makefile
+++ b/drivers/power/asv/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_POWER_ASV)			+= asv.o
+obj-$(CONFIG_POWER_EXYNOS_ASV)		+= exynos-asv.o
diff --git a/drivers/power/asv/exynos-asv.c b/drivers/power/asv/exynos-asv.c
new file mode 100644
index 000000000000..50efb0b6af85
--- /dev/null
+++ b/drivers/power/asv/exynos-asv.c
@@ -0,0 +1,70 @@
+/* Common Exynos ASV(Adaptive Supply Voltage) driver
+ *
+ * Copyright (c) 2013 Samsung Electronics Co., Ltd.
+ *		http://www.samsung.com/
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+*/
+
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/of_address.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/power/asv-driver.h>
+#include "exynos-asv.h"
+
+static int exynos_asv_probe(struct platform_device *pdev)
+{
+	struct device_node *chip_id;
+	struct exynos_asv_common *exynos_asv_info;
+	void __iomem *base;
+	int ret = 0;
+
+	exynos_asv_info = devm_kzalloc(&pdev->dev, sizeof(*exynos_asv_info),
+				       GFP_KERNEL);
+	if (!exynos_asv_info)
+		return -ENOMEM;
+
+	chip_id = of_find_compatible_node(NULL, NULL,
+						"samsung,exynos4210-chipid");
+	if (!chip_id) {
+		pr_err("%s: unable to find chipid\n", __func__);
+		return -ENODEV;
+	}
+
+	base = of_iomap(chip_id, 0);
+	if (!base) {
+		dev_err(&pdev->dev, "unable to map chip_id register\n");
+		ret = -ENOMEM;
+		goto err_map;
+	}
+
+	exynos_asv_info->base = base;
+
+	/* call SoC specific intialisation routine */
+
+	register_asv_member(exynos_asv_info->asv_list, exynos_asv_info->nr_mem);
+
+	iounmap(base);
+err_map:
+	of_node_put(chip_id);
+
+	return ret;
+}
+
+static struct platform_driver exynos_asv_platdrv = {
+	.driver = {
+		.name	= "exynos-asv",
+		.owner	= THIS_MODULE,
+	},
+	.probe		= exynos_asv_probe,
+};
+module_platform_driver(exynos_asv_platdrv);
+
+MODULE_AUTHOR("Yadwinder Singh Brar<yadi.brar@samsung.com>");
+MODULE_DESCRIPTION("Exynos ASV driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/power/asv/exynos-asv.h b/drivers/power/asv/exynos-asv.h
new file mode 100644
index 000000000000..89a1ae8b5e19
--- /dev/null
+++ b/drivers/power/asv/exynos-asv.h
@@ -0,0 +1,21 @@
+/*
+ * Exynos - Adaptive Supply Voltage Driver Header File
+ *
+ * copyright (c) 2013 samsung electronics co., ltd.
+ *		http://www.samsung.com/
+ *
+ * this program is free software; you can redistribute it and/or modify
+ * it under the terms of the gnu general public license version 2 as
+ * published by the free software foundation.
+*/
+
+#ifndef __EXYNOS_ASV_D_H
+#define __EXYNOS_ASV_D_H __FILE__
+
+struct exynos_asv_common {
+	struct asv_info *asv_list;
+	unsigned int nr_mem;
+	void __iomem *base;
+};
+
+#endif	/* __EXYNOS_ASV_D_H */
-- 
1.7.9.5

[RFC v2 3/4] power: asv: Add support for Exynos5250

[Sachin Kamat]

From: Yadwinder Singh Brar <yadi.brar@samsung.com>

This patch adds basic support (only for ARM ASV) for exynos5250 chips
which have fused ASV group.

Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
---
 drivers/power/asv/Makefile         |    2 +-
 drivers/power/asv/exynos-asv.c     |    8 +++
 drivers/power/asv/exynos-asv.h     |    1 +
 drivers/power/asv/exynos5250-asv.c |  139 ++++++++++++++++++++++++++++++++++++
 4 files changed, 149 insertions(+), 1 deletion(-)
 create mode 100644 drivers/power/asv/exynos5250-asv.c

diff --git a/drivers/power/asv/Makefile b/drivers/power/asv/Makefile
index 9d0e0cc32cb5..a73544445c9a 100644
--- a/drivers/power/asv/Makefile
+++ b/drivers/power/asv/Makefile
@@ -1,2 +1,2 @@
 obj-$(CONFIG_POWER_ASV)			+= asv.o
-obj-$(CONFIG_POWER_EXYNOS_ASV)		+= exynos-asv.o
+obj-$(CONFIG_POWER_EXYNOS_ASV)		+= exynos-asv.o exynos5250-asv.o
diff --git a/drivers/power/asv/exynos-asv.c b/drivers/power/asv/exynos-asv.c
index 50efb0b6af85..f2cd04d1ce95 100644
--- a/drivers/power/asv/exynos-asv.c
+++ b/drivers/power/asv/exynos-asv.c
@@ -46,9 +46,17 @@ static int exynos_asv_probe(struct platform_device *pdev)
 	exynos_asv_info->base = base;
 
 	/* call SoC specific intialisation routine */
+	if (of_machine_is_compatible("samsung,exynos5250")) {
+		ret = exynos5250_asv_init(exynos_asv_info);
+		if (ret) {
+			pr_err("exynos5250_asv_init failed : %d\n", ret);
+			goto err;
+		}
+	}
 
 	register_asv_member(exynos_asv_info->asv_list, exynos_asv_info->nr_mem);
 
+err:
 	iounmap(base);
 err_map:
 	of_node_put(chip_id);
diff --git a/drivers/power/asv/exynos-asv.h b/drivers/power/asv/exynos-asv.h
index 89a1ae8b5e19..a31becb7478f 100644
--- a/drivers/power/asv/exynos-asv.h
+++ b/drivers/power/asv/exynos-asv.h
@@ -18,4 +18,5 @@ struct exynos_asv_common {
 	void __iomem *base;
 };
 
+extern int exynos5250_asv_init(struct exynos_asv_common *exynos_info);
 #endif	/* __EXYNOS_ASV_D_H */
diff --git a/drivers/power/asv/exynos5250-asv.c b/drivers/power/asv/exynos5250-asv.c
new file mode 100644
index 000000000000..7746c7d2d987
--- /dev/null
+++ b/drivers/power/asv/exynos5250-asv.c
@@ -0,0 +1,139 @@
+/* exynos5250 - ASV(Adaptive Supply Voltage)
+ *
+ * Copyright (c) 2013 Samsung Electronics Co., Ltd.
+ *		http://www.samsung.com/
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+*/
+
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/power/asv-driver.h>
+#include "exynos-asv.h"
+
+#define FUSED_SG_OFFSET		3
+#define ORIG_SG_OFFSET		17
+#define ORIG_SG_MASK		0xF
+#define MOD_SG_OFFSET		21
+#define MOD_SG_MASK		0x7
+
+#define ARM_LEVEL_NR		16
+#define ARM_GRP_NR		12
+
+#define CHIP_ID_OFFSET		0x4
+
+struct exynos5250_asv_info {
+	unsigned int package_id;
+	/* we may need more info as global data */
+};
+
+static struct exynos5250_asv_info asv_group __initdata;
+
+static unsigned int asv_voltage[ARM_LEVEL_NR][ARM_GRP_NR + 1] __initdata = {
+	{ 1700000, 1300000, 1275000, 1275000, 1262500, 1250000, 1225000,
+	1212500, 1200000, 1187500, 1175000, 1150000, 1125000 },	/* L0 */
+	{ 1600000, 1250000, 1225000, 1225000, 1212500, 1200000, 1187500,
+	1175000, 1162500, 1150000, 1137500, 1112500, 1100000 },	/* L1 */
+	{ 1500000, 1225000, 1187500, 1175000, 1162500, 1150000, 1137500,
+	1125000, 1112500, 1100000, 1087500, 1075000, 1062500 },	/* L2 */
+	{ 1400000, 1200000, 1125000, 1125000, 1125000, 1112500, 1100000,
+	1087500, 1075000, 1062500, 1050000, 1037500, 1025000 },	/* L3 */
+	{ 1300000, 1150000, 1100000, 1100000, 1100000, 1087500, 1075000,
+	1062500, 1050000, 1037500, 1025000, 1012500, 1000000 },	/* L4 */
+	{ 1200000, 1125000, 1075000, 1075000, 1062500, 1050000, 1037500,
+	1025000, 1012500, 1000000,  987500,  975000,  975000 },	/* L5 */
+	{ 1100000, 1100000, 1050000, 1050000, 1037500, 1025000, 1012500,
+	1000000,  987500,  975000,  962500,  950000,  925000 },	/* L6 */
+	{ 1000000, 1075000, 1037500, 1037500, 1012500, 1000000,  987500,
+	975000,  962500,  950000,  937500,  925000,  912500 },	/* L7 */
+	{ 900000, 1050000, 1025000, 1012500,  987500,  975000,  962500,
+	950000,  937500,  925000,  912500,  912500,  900000 },	/* L8 */
+	{ 800000, 1025000, 1000000,  987500,  975000,  962500,  950000,
+	937500,  925000,  912500,  900000,  900000,  900000 },	/* L9 */
+	{ 700000, 1012500,  975000,  962500,  950000,  937500,  925000,
+	912500,  900000,  900000,  900000,  900000,  900000 },	/* L10 */
+	{ 600000, 1000000,  962500,  950000,  937500,  925000,  912500,
+	900000,  900000,  900000,  900000,  900000,  900000 },	/* L11 */
+	{ 500000, 975000,  950000,  937500,  925000,  912500,  900000,
+	900000,  900000,  900000,  900000,  900000,  887500 },	/* L12 */
+	{ 400000, 950000,  937500,  925000,  912500,  900000,  900000,
+	900000,  900000,  900000,  900000,  887500,  887500 },	/* L13 */
+	{ 300000, 937500,  925000,  912500,  900000,  900000,  900000,
+	900000,  900000,  900000,  887500,  887500,  875000 },	/* L14 */
+	{ 200000, 925000,  912500,  900000,  900000,  900000,  900000,
+	900000,  900000,  887500,  887500,  875000,  875000 },	/* L15 */
+};
+
+static int __init exynos5250_get_asv_group(struct asv_info *asv_info)
+{
+	int exynos_asv_grp;
+	u32 exynos_orig_sp;
+	u32 exynos_mod_sp;
+	u32 package_id = asv_group.package_id;
+
+	/* If ASV group is fused then retrieve it */
+	if ((package_id >> FUSED_SG_OFFSET) & 0x1) {
+		exynos_orig_sp = (package_id >> ORIG_SG_OFFSET) & ORIG_SG_MASK;
+		exynos_mod_sp = (package_id >> MOD_SG_OFFSET) & MOD_SG_MASK;
+
+		exynos_asv_grp = exynos_orig_sp - exynos_mod_sp;
+		if (exynos_asv_grp < 0) {
+			pr_warn("%s: Invalid ASV group: %d\n", __func__,
+				exynos_asv_grp);
+			exynos_asv_grp = 0;	/* go for default */
+		}
+	} else {
+		pr_warn("%s: ASV group not fused for : %s\n", __func__,
+			asv_info->name);
+		exynos_asv_grp = 0;		/* go for default */
+	}
+
+	asv_info->asv_grp = exynos_asv_grp;
+	return 0;
+}
+
+static int __init exynos5250_init_arm_asv_table(struct asv_info *asv_info)
+{
+	struct asv_freq_table *dvfs_table;
+	int i, asv_grp = asv_info->asv_grp;
+
+	dvfs_table = kzalloc(sizeof(*dvfs_table) * ARM_LEVEL_NR, GFP_KERNEL);
+	if (!dvfs_table)
+		return -ENOMEM;
+
+	for (i = 0; i < ARM_LEVEL_NR; i++) {
+		dvfs_table[i].freq = asv_voltage[i][0];
+		dvfs_table[i].volt = asv_voltage[i][asv_grp + 1];
+	}
+
+	asv_info->dvfs_table = dvfs_table;
+	return 0;
+}
+
+/* TODO: Implement .init_asv callback to set ABB value */
+
+static struct asv_ops exynos5250_arm_asv_ops __initdata = {
+	.get_asv_group = exynos5250_get_asv_group,
+	.init_asv_table = exynos5250_init_arm_asv_table,
+};
+
+static struct asv_info exynos5250_asv_member[] __initdata = {
+	{
+		.type		= ASV_ARM,
+		.name		= "VDD_ARM",
+		.ops		= &exynos5250_arm_asv_ops,
+		.nr_dvfs_level	= ARM_LEVEL_NR,
+	},
+};
+
+int __init exynos5250_asv_init(struct exynos_asv_common *exynos_info)
+{
+	asv_group.package_id = readl(exynos_info->base + CHIP_ID_OFFSET);
+	exynos_info->asv_list = exynos5250_asv_member;
+	exynos_info->nr_mem = ARRAY_SIZE(exynos5250_asv_member);
+
+	return 0;
+}
-- 
1.7.9.5

[RFC v2 4/4] ARM: SAMSUNG: Register static platform device for ASV for Exynos5

[Sachin Kamat]

From: Yadwinder Singh Brar <yadi.brar@samsung.com>

Since ASV is not a hardware controller, we can't add device tree node
for it. This patch registers a static platform device for Exynos ASV.

Signed-off-by: Yadwinder Singh Brar <yadi.brar@samsung.com>
Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
---
 arch/arm/mach-exynos/mach-exynos5-dt.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/mach-exynos/mach-exynos5-dt.c b/arch/arm/mach-exynos/mach-exynos5-dt.c
index 602c5d7111d0..831b7222aab6 100644
--- a/arch/arm/mach-exynos/mach-exynos5-dt.c
+++ b/arch/arm/mach-exynos/mach-exynos5-dt.c
@@ -24,6 +24,7 @@ static void __init exynos5_dt_machine_init(void)
 	struct device_node *i2c_np;
 	const char *i2c_compat = "samsung,s3c2440-i2c";
 	unsigned int tmp;
+	struct platform_device_info devinfo = { .name = "exynos-asv", };
 
 	/*
 	 * Exynos5's legacy i2c controller and new high speed i2c
@@ -47,6 +48,7 @@ static void __init exynos5_dt_machine_init(void)
 	exynos_cpufreq_init();
 
 	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
+	platform_device_register_full(&devinfo);
 }
 
 static char const *exynos5_dt_compat[] __initdata = {
-- 
1.7.9.5

Re: [RFC v2 0/4] Add basic support for ASV

[Sachin Kamat]

Hi MyungJoo,

On 18 November 2013 08:07, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
>
> 2013. 11. 15. 오후 8:44에 "Sachin Kamat" <sachin.kamat@linaro.org>님이 작성:
>
>
>>
>> Original cover letter from Yadwinder:
>> This series is to add basic common infrastructure for ASV.
>>  Basically ASV is a technique used on samsung SoCs, which provides the
>> recommended supply voltage for dvfs of arm, mif etc. For a given operating
>> frequency, the voltage is recommended based on SoC's ASV group.
>> ASV group gets fussed on SoCs during process of mass production.
>>
>> This series includes:
>>  - basic common infrastructue for ASV. It provides common APIs for user
>> drivers
>> like cpufreq & devfreq and and an interface for SoC specific drivers to
>> register ASV members(instances)
>>  - a common platform driver to register ASV members for exynos SoCs
>>  - an example providing minimal support (only for ARM ASV) for exynos5250
>> chips
>>
>> Its just basic skelton which I wanted to get it reviewed or discussed in
>> early stage, before going ahead on further development based on it.
>>  Presently example is based on static ASV table provided in SoC specific
>> file,
>> which I expects to go into DT. But exactly how and where needs to be
>> discussed,
>> may be in next revisions once we get through the basic skelton.
>>  Also the location of driver in kernel may also seem odd to someone and
>> many more things :).
>>
>> Looking for your valuable reviews and suggestions.
>
> Hi,
>
> As I have commented on the previous thread of Yadwinder, please share the
> directory woth AVS, which is conceptually a superset of ASV.
>
> Ultimately, it would be best if you can supply an infra that can be shared
> with the current AVS, which does not have any currently.
>

Yadwinder has already replied to your suggestion about his concerns. Please
share your comments.

-- 
With warm regards,
Sachin

Re: [RFC v2 0/4] Add basic support for ASV

[Abhilash Kesavan]

Hi Yadwinder and Sachin,

On Mon, Nov 18, 2013 at 9:37 AM, Sachin Kamat <sachin.kamat@linaro.org> wrote:
> Hi MyungJoo,
>
> On 18 November 2013 08:07, MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
>>
>> 2013. 11. 15. 오후 8:44에 "Sachin Kamat" <sachin.kamat@linaro.org>님이 작성:
>>
>>
>>>
>>> Original cover letter from Yadwinder:
>>> This series is to add basic common infrastructure for ASV.
>>>  Basically ASV is a technique used on samsung SoCs, which provides the
>>> recommended supply voltage for dvfs of arm, mif etc. For a given operating
>>> frequency, the voltage is recommended based on SoC's ASV group.
>>> ASV group gets fussed on SoCs during process of mass production.
>>>
>>> This series includes:
>>>  - basic common infrastructue for ASV. It provides common APIs for user
>>> drivers
>>> like cpufreq & devfreq and and an interface for SoC specific drivers to
>>> register ASV members(instances)
>>>  - a common platform driver to register ASV members for exynos SoCs
>>>  - an example providing minimal support (only for ARM ASV) for exynos5250
>>> chips
>>>
>>> Its just basic skelton which I wanted to get it reviewed or discussed in
>>> early stage, before going ahead on further development based on it.
>>>  Presently example is based on static ASV table provided in SoC specific
>>> file,
>>> which I expects to go into DT. But exactly how and where needs to be
>>> discussed,
>>> may be in next revisions once we get through the basic skelton.
>>>  Also the location of driver in kernel may also seem odd to someone and
>>> many more things :).
>>>
>>> Looking for your valuable reviews and suggestions.
>>
>> Hi,
>>
>> As I have commented on the previous thread of Yadwinder, please share the
>> directory woth AVS, which is conceptually a superset of ASV.
>>
>> Ultimately, it would be best if you can supply an infra that can be shared
>> with the current AVS, which does not have any currently.
>>
>
> Yadwinder has already replied to your suggestion about his concerns. Please
> share your comments.
>

CC'ing Doug and Andrew who have also worked on ASV.

I tested these patches on a 5250 Chromebook after modifying the
cpufreq code and a few other changes for booting the board. The driver
is retrieving the ASV fused group correctly. The behavior on an
unfused SMDK5250 is also fine.
I have a few minor comments on the patches.

Thanks
Abhilash

Re: [RFC v2 1/4] power: asv: Add common ASV support for Samsung SoCs

[Abhilash Kesavan]

Hi,

CC'ing Doug and Andrew who have also worked on ASV.

[...]

> diff --git a/drivers/power/asv/Kconfig b/drivers/power/asv/Kconfig
> new file mode 100644
> index 000000000000..761119d9f7f8
> --- /dev/null
> +++ b/drivers/power/asv/Kconfig
> @@ -0,0 +1,10 @@
> +menuconfig POWER_ASV
> +       bool "Adaptive Supply Voltage (ASV) support"
Select POWER_SUPPLY here ?
> +       help
> +         ASV is a technique used on Samsung SoCs which provides the
> +         recommended supply voltage for some specific parts(like CPU, MIF, etc)
> +         that support DVFS. For a given operating frequency, the voltage is
> +         recommended based on SoCs ASV group. ASV group info is provided in the
> +         chip id info which depends on the chip manufacturing process.
> +

[...]
> +
> +       if (asv_info->ops->init_asv)
> +               ret = asv_info->ops->init_asv(asv_info);
> +               if (ret) {
> +                       pr_err("asv_init failed for %s : %d\n",
> +                               asv_info->name, ret);
> +                       goto err;
> +               }
> +
> +       /* In case of parsing table from DT, we may need to add flag to identify
> +       DT supporting members and call init_asv_table from asv_init_opp_table(
> +       after getting dev_node from dev,if required), instead of calling here.
> +       */
Please fix Multi-line comment here and through the rest of the patches as well.

[...]
> + * @nr_dvfs_level: Number of dvfs levels supported by member.
> + * @dvfs_table: Table containing supported ASV freqs and corresponding volts.
> + * @asv_grp: ASV group of member.
> + * @flags: ASV flags
What are the ASV flags you had in mind ?
> + */
> +struct asv_info {
> +       const char              *name;
> +       enum asv_type_id        type;
> +       struct asv_ops          *ops;
> +       unsigned int            nr_dvfs_level;
> +       struct asv_freq_table   *dvfs_table;
> +       unsigned int            asv_grp;
> +       unsigned int            flags;
> +};

[...]
> +
> +#ifdef CONFIG_POWER_ASV
> +/* asv_get_volt - get the ASV for target_freq for particular target_type.
> + *     returns 0 if target_freq is not supported
Could you add a comment for asv_init_opp_table as well.
> + */
> +extern unsigned int asv_get_volt(enum asv_type_id target_type,
> +                                       unsigned int target_freq);
> +extern int asv_init_opp_table(struct device *dev,
> +                                       enum asv_type_id target_type);
> +#else
> +static inline unsigned int asv_get_volt(enum asv_type_id target_type,
> +                               unsigned int target_freq) { return 0; }
> +static int asv_init_opp_table(struct device *dev, enum asv_type_id target_type)
> +       { return 0; }
> +
> +#endif /* CONFIG_POWER_EXYNOS_AVS */
> +#endif /* __ASV_H */

Regards,
Abhilash

Re: [RFC v2 2/4] power: asv: Add a common ASV driver for Exynos SoCs.

[Abhilash Kesavan]

Hi,

CC'ing Doug and Andrew who have also worked on ASV.

[...]

> +
> +       chip_id = of_find_compatible_node(NULL, NULL,
> +                                               "samsung,exynos4210-chipid");
> +       if (!chip_id) {
> +               pr_err("%s: unable to find chipid\n", __func__);
> +               return -ENODEV;
> +       }
> +
> +       base = of_iomap(chip_id, 0);
> +       if (!base) {
> +               dev_err(&pdev->dev, "unable to map chip_id register\n");
> +               ret = -ENOMEM;
> +               goto err_map;
> +       }
My u-boot had the chipid clock disabled and I was getting an invalid
value at 1000_0004 (0 on the SMDK5420 and 0x43520010 on the
chromebook). Maybe it would be better if we enable the chipid clock in
the driver as well.
> +
> +       exynos_asv_info->base = base;
> +
> +       /* call SoC specific intialisation routine */
> +
> +       register_asv_member(exynos_asv_info->asv_list, exynos_asv_info->nr_mem);
> +
> +       iounmap(base);
> +err_map:
> +       of_node_put(chip_id);
> +
> +       return ret;
> +}
[...]
> diff --git a/drivers/power/asv/exynos-asv.h b/drivers/power/asv/exynos-asv.h
> new file mode 100644
> index 000000000000..89a1ae8b5e19
> --- /dev/null
> +++ b/drivers/power/asv/exynos-asv.h
> @@ -0,0 +1,21 @@
> +/*
> + * Exynos - Adaptive Supply Voltage Driver Header File
> + *
> + * copyright (c) 2013 samsung electronics co., ltd.
> + *             http://www.samsung.com/
Please fix the copyright

Thanks,
Abhilash

Re: [RFC v2 0/4] Add basic support for ASV

[Sachin Kamat]

Hi Abhilash,

On 3 December 2013 20:16, Abhilash Kesavan <kesavan.abhilash@gmail.com> wrote:
> Hi Yadwinder and Sachin,

> CC'ing Doug and Andrew who have also worked on ASV.
>
> I tested these patches on a 5250 Chromebook after modifying the
> cpufreq code and a few other changes for booting the board. The driver
> is retrieving the ASV fused group correctly. The behavior on an
> unfused SMDK5250 is also fine.
> I have a few minor comments on the patches.
>

Thank you for testing and reviewing the patchset.
Will incorporate your comments in the next version.

-- 
With warm regards,
Sachin

Re: [RFC v2 1/4] power: asv: Add common ASV support for Samsung SoCs

[Tomasz Figa]

Hi Yadwinder, Sachin,

On Friday 15 of November 2013 17:11:28 Sachin Kamat wrote:
> From: Yadwinder Singh Brar <yadi.brar@samsung.com>
> 
> This patch introduces a common ASV (Adaptive Supply Voltage) basic framework
> for samsung SoCs. It provides common APIs (to be called by users to get ASV
> values or init opp_table) and an interface for SoC specific drivers to
> register ASV members (instances).
[snip]
> diff --git a/drivers/power/asv/asv.c b/drivers/power/asv/asv.c
> new file mode 100644
> index 000000000000..3f2c31a0d3a9
> --- /dev/null
> +++ b/drivers/power/asv/asv.c
> @@ -0,0 +1,176 @@
> +/*
> + * ASV(Adaptive Supply Voltage) common core
> + *
> + * Copyright (c) 2013 Samsung Electronics Co., Ltd.
> + *		http://www.samsung.com/
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> +*/
> +
> +#include <linux/io.h>
> +#include <linux/pm_opp.h>
> +#include <linux/slab.h>
> +#include <linux/device.h>
> +#include <linux/power/asv-driver.h>
> +
> +static LIST_HEAD(asv_list);
> +static DEFINE_MUTEX(asv_mutex);
> +
> +struct asv_member {
> +	struct list_head		node;
> +	struct asv_info *asv_info;

nit: Inconsistent indentation of member names. In general I would
recommend dropping the tabs between types and names and using a single
space instead, since this is more future proof - you will never have to
change other lines to add new ones.

> +};
> +
> +static void add_asv_member(struct asv_member *asv_mem)
> +{
> +	mutex_lock(&asv_mutex);
> +	list_add_tail(&asv_mem->node, &asv_list);
> +	mutex_unlock(&asv_mutex);
> +}
> +
> +static struct asv_member *asv_get_mem(enum asv_type_id asv_type)

I don't really like this enum based look-up. It's hard to define an enum
that covers any possible existing and future platforms that would not be
bloated with single platform specific entries. IMHO something string based
could be more scalable.

> +{
> +	struct asv_member *asv_mem;
> +	struct asv_info *asv_info;
> +
> +	list_for_each_entry(asv_mem, &asv_list, node) {
> +		asv_info = asv_mem->asv_info;
> +		if (asv_type == asv_info->type)
> +			return asv_mem;
> +	}

Don't you need any kind of locking here? A mutex in add_asv_member()
suggests that read access to the list should be protected as well.

> +
> +	return NULL;
> +}
> +
> +unsigned int asv_get_volt(enum asv_type_id target_type,
> +						unsigned int target_freq)

Do you need this function at all? I believe this is all about populating
device's OPP array with frequencies and voltages according to its ASV
level. Users will be able to query for required voltage using standard OPP
calls then, without a need for ASV specific functions like this one.

> +{
> +	struct asv_member *asv_mem = asv_get_mem(target_type);
> +	struct asv_freq_table *dvfs_table;
> +	struct asv_info *asv_info;
> +	unsigned int i;
> +
> +	if (!asv_mem)
> +		return 0;
> +
> +	asv_info = asv_mem->asv_info;
> +	dvfs_table = asv_info->dvfs_table;
> +
> +	for (i = 0; i < asv_info->nr_dvfs_level; i++) {
> +		if (dvfs_table[i].freq == target_freq)
> +			return dvfs_table[i].volt;
> +	}
> +
> +	return 0;
> +}
> +
> +int asv_init_opp_table(struct device *dev, enum asv_type_id target_type)
> +{
> +	struct asv_member *asv_mem = asv_get_mem(target_type);
> +	struct asv_info *asv_info;
> +	struct asv_freq_table *dvfs_table;
> +	unsigned int i;
> +
> +	if (!asv_mem)
> +		return -EINVAL;
> +
> +	asv_info = asv_mem->asv_info;
> +	dvfs_table = asv_info->dvfs_table;
> +
> +	for (i = 0; i < asv_info->nr_dvfs_level; i++) {
> +		if (dev_pm_opp_add(dev, dvfs_table[i].freq * 1000,
> +			dvfs_table[i].volt)) {
> +			dev_warn(dev, "Failed to add OPP %d\n",
> +				 dvfs_table[i].freq);

Hmm, shouldn't it be considered a failure instead?

> +			continue;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static struct asv_member *asv_init_member(struct asv_info *asv_info)
> +{
> +	struct asv_member *asv_mem;
> +	int ret = 0;
> +
> +	if (!asv_info) {
> +		pr_err("No ASV info provided\n");
> +		return NULL;

I'd suggest adopting the ERR_PTR() convention, which allows returning more
information about the error.

> +	}
> +
> +	asv_mem = kzalloc(sizeof(*asv_mem), GFP_KERNEL);
> +	if (!asv_mem) {
> +		pr_err("Allocation failed for member: %s\n", asv_info->name);
> +		return NULL;
> +	}
> +
> +	asv_mem->asv_info = kmemdup(asv_info, sizeof(*asv_info), GFP_KERNEL);
> +	if (!asv_mem->asv_info) {
> +		pr_err("Copying asv_info failed for member: %s\n",
> +			asv_info->name);
> +		kfree(asv_mem);
> +		return NULL;

For consistency, the error here should be handled as below, by using the
error path.

> +	}
> +	asv_info = asv_mem->asv_info;
> +
> +	if (asv_info->ops->get_asv_group) {
> +		ret = asv_info->ops->get_asv_group(asv_info);
> +		if (ret) {
> +			pr_err("get_asv_group failed for %s : %d\n",
> +				asv_info->name, ret);
> +			goto err;
> +		}
> +	}
> +
> +	if (asv_info->ops->init_asv)
> +		ret = asv_info->ops->init_asv(asv_info);
> +		if (ret) {
> +			pr_err("asv_init failed for %s : %d\n",
> +				asv_info->name, ret);
> +			goto err;
> +		}
> +
> +	/* In case of parsing table from DT, we may need to add flag to identify
> +	DT supporting members and call init_asv_table from asv_init_opp_table(
> +	after getting dev_node from dev,if required), instead of calling here.
> +	*/

coding style: Wrong multi-line comment style.

/*
 * This is a valid multi-line comment.
 */

> +
> +	if (asv_info->ops->init_asv_table) {
> +		ret = asv_info->ops->init_asv_table(asv_info);
> +		if (ret) {
> +			pr_err("init_asv_table failed for %s : %d\n",
> +				asv_info->name, ret);
> +			goto err;
> +		}
> +	}

Hmm, I don't see a point of these three separate callbacks above.

In general, I'd suggest a different architecture. I'd see this more as:

1) Platform code registers static platform device to instantiate SoC ASV
   driver.
2) SoC specific ASV driver probes, reads group ID from hardware register,
   calls register_asv_member() with appropriate DVFS table for detected
   group.
3) Driver using ASV calls asv_init_opp_table() with its struct device and
   ASV member name, which causes the ASV code to fill device's operating
   point using OPP calls.

Now client driver has all the information it needs and the work of ASV
subsystem is done. The control flow between drivers would be much simpler
and no callbacks would have to be called.

Best regards,
Tomasz

Re: [RFC v2 1/4] power: asv: Add common ASV support for Samsung SoCs

[Yadwinder Singh Brar]

Hi Tomasz,

Thanks for your thorough review and nice suggestions.

[snip]
>> +}
>> +
>> +static struct asv_member *asv_get_mem(enum asv_type_id asv_type)
>
> I don't really like this enum based look-up. It's hard to define an enum
> that covers any possible existing and future platforms that would not be
> bloated with single platform specific entries. IMHO something string based
> could be more scalable.
>

Yes, I also agree string based look-up will be better. I was thinking
to convert to it,
after initial discussion over the APIs.

>> +{
>> +     struct asv_member *asv_mem;
>> +     struct asv_info *asv_info;
>> +
>> +     list_for_each_entry(asv_mem, &asv_list, node) {
>> +             asv_info = asv_mem->asv_info;
>> +             if (asv_type == asv_info->type)
>> +                     return asv_mem;
>> +     }
>
> Don't you need any kind of locking here? A mutex in add_asv_member()
> suggests that read access to the list should be protected as well.
>

hmmm, yes should be their for completeness of code.

>> +
>> +     return NULL;
>> +}
>> +
>> +unsigned int asv_get_volt(enum asv_type_id target_type,
>> +                                             unsigned int target_freq)
>
> Do you need this function at all? I believe this is all about populating
> device's OPP array with frequencies and voltages according to its ASV
> level. Users will be able to query for required voltage using standard OPP
> calls then, without a need for ASV specific functions like this one.
>

Yes, I had put a comment in initial version after commit message :
"Hopefully asv_get_volt() can go out in future, once all users start using OPP
library." , which seems to be missed in this version.
I had kept it for the time being in initial version, to keep it
usable(for testing) with
existing cpufreq drivers, which need to reworked and may take time.

[snip]
>> +
>> +     for (i = 0; i < asv_info->nr_dvfs_level; i++) {
>> +             if (dev_pm_opp_add(dev, dvfs_table[i].freq * 1000,
>> +                     dvfs_table[i].volt)) {
>> +                     dev_warn(dev, "Failed to add OPP %d\n",
>> +                              dvfs_table[i].freq);
>
> Hmm, shouldn't it be considered a failure instead?
>

hmm, not really always. Theoretically system with some less(failed to add)
levels can work. Moreover I had prefered to keep it only warning, just to
keep the behaviour of  asv_init_opp_table() similar to that of its
counter part of_init_opp_table().

>> +                     continue;
>> +             }
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static struct asv_member *asv_init_member(struct asv_info *asv_info)
>> +{
>> +     struct asv_member *asv_mem;
>> +     int ret = 0;
>> +
>> +     if (!asv_info) {
>> +             pr_err("No ASV info provided\n");
>> +             return NULL;
>
> I'd suggest adopting the ERR_PTR() convention, which allows returning more
> information about the error.
>

Will it be really usefull here?, as we are not checking return value
of any function.
Bur for some cases below, i will also like to get it used.

>> +     }
>> +
>> +     asv_mem = kzalloc(sizeof(*asv_mem), GFP_KERNEL);
>> +     if (!asv_mem) {
>> +             pr_err("Allocation failed for member: %s\n", asv_info->name);
>> +             return NULL;
>> +     }

[snip]
>
> Hmm, I don't see a point of these three separate callbacks above.
>
> In general, I'd suggest a different architecture. I'd see this more as:
>
> 1) Platform code registers static platform device to instantiate SoC ASV
>    driver.
> 2) SoC specific ASV driver probes, reads group ID from hardware register,
>    calls register_asv_member() with appropriate DVFS table for detected
>    group.
> 3) Driver using ASV calls asv_init_opp_table() with its struct device and
>    ASV member name, which causes the ASV code to fill device's operating
>    point using OPP calls.
>
> Now client driver has all the information it needs and the work of ASV
> subsystem is done. The control flow between drivers would be much simpler
> and no callbacks would have to be called.
>

Architecture stated above seems to be a subset(one possible way of use),
of the proposed architecture. If someone really have nothing much to do,
he can adopt the above stated approach using this framework also,
callbacks are not mandatory.

Since we usually have more things to do other than only reading
fused group value and simply parsing a table index, so in drivers we have to
implement functions to segregate stuff and different people do it in
different way. Its an attempt to provide a way to keep structure(functions)
similar for easy understanding and factoring out of common code.

Moreover, I feels need of callbacks if we have to do something depending
upon(specific) the user/instance of  ASV member. One thing came
in my mind was dev_node may be required if we may think of parsing
ASV table from DT and may be more things in future.


I would like to get rectified, other nit/suggestions stated by you in
next version.


Thanks & Regards,
 Yadwinder

Re: [RFC v2 1/4] power: asv: Add common ASV support for Samsung SoCs

[Yadwinder Singh Brar]

Hi Abhilash,

[ ... ]
>> + * @nr_dvfs_level: Number of dvfs levels supported by member.
>> + * @dvfs_table: Table containing supported ASV freqs and corresponding volts.
>> + * @asv_grp: ASV group of member.
>> + * @flags: ASV flags
> What are the ASV flags you had in mind ?

Right now we don't have any, some thing like delayed
init of asv table depending upon dev_node/user(instance)
was coming in my mind.
Actually I missed to remove it for the time being.

Thanks for your review and other suggestions.

Regards,
Yadwinder

Re: [RFC v2 1/4] power: asv: Add common ASV support for Samsung SoCs

[Tomasz Figa]

On Sunday 15 of December 2013 22:30:13 Yadwinder Singh Brar wrote:
[snip]
> >> +
> >> +     return NULL;
> >> +}
> >> +
> >> +unsigned int asv_get_volt(enum asv_type_id target_type,
> >> +                                             unsigned int target_freq)
> >
> > Do you need this function at all? I believe this is all about populating
> > device's OPP array with frequencies and voltages according to its ASV
> > level. Users will be able to query for required voltage using standard OPP
> > calls then, without a need for ASV specific functions like this one.
> >
> 
> Yes, I had put a comment in initial version after commit message :
> "Hopefully asv_get_volt() can go out in future, once all users start using OPP
> library." , which seems to be missed in this version.
> I had kept it for the time being in initial version, to keep it
> usable(for testing) with
> existing cpufreq drivers, which need to reworked and may take time.

Hmm, at the moment none of cpufreq drivers use ASV, so they need to be
reworked anyway to use it either by the means of a private get_volt
function or OPP framework. I agree that OPP may require more work,
though.

If we decide to keep this function in final version, a comment should be
added saying that its usage is deprecated in favor of generic OPP helpers.

> 
> [snip]
> >> +
> >> +     for (i = 0; i < asv_info->nr_dvfs_level; i++) {
> >> +             if (dev_pm_opp_add(dev, dvfs_table[i].freq * 1000,
> >> +                     dvfs_table[i].volt)) {
> >> +                     dev_warn(dev, "Failed to add OPP %d\n",
> >> +                              dvfs_table[i].freq);
> >
> > Hmm, shouldn't it be considered a failure instead?
> >
> 
> hmm, not really always. Theoretically system with some less(failed to add)
> levels can work. Moreover I had prefered to keep it only warning, just to
> keep the behaviour of  asv_init_opp_table() similar to that of its
> counter part of_init_opp_table().

I'm not quite convinced about it. If dev_pm_opp_add() fails, doesn't it mean
that something broke seriously in upper layer and we should propagate the
error down? Especially when looking at opp_add(), the only failure
conditions I can find are memory allocation errors which mean that the
system is unlikely to operate correctly anyway.

> 
> >> +                     continue;
> >> +             }
> >> +     }
> >> +
> >> +     return 0;
> >> +}
> >> +
> >> +static struct asv_member *asv_init_member(struct asv_info *asv_info)
> >> +{
> >> +     struct asv_member *asv_mem;
> >> +     int ret = 0;
> >> +
> >> +     if (!asv_info) {
> >> +             pr_err("No ASV info provided\n");
> >> +             return NULL;
> >
> > I'd suggest adopting the ERR_PTR() convention, which allows returning more
> > information about the error.
> >
> 
> Will it be really usefull here?, as we are not checking return value
> of any function.

Why not? Here you have ERR_PTR(-EINVAL), then...

> Bur for some cases below, i will also like to get it used.
> 
> >> +     }
> >> +
> >> +     asv_mem = kzalloc(sizeof(*asv_mem), GFP_KERNEL);
> >> +     if (!asv_mem) {
> >> +             pr_err("Allocation failed for member: %s\n", asv_info->name);
> >> +             return NULL;

ERR_PTR(-ENOMEM) here.

These are two completely different error cases.

> >> +     }
> 
> [snip]
> >
> > Hmm, I don't see a point of these three separate callbacks above.
> >
> > In general, I'd suggest a different architecture. I'd see this more as:
> >
> > 1) Platform code registers static platform device to instantiate SoC ASV
> >    driver.
> > 2) SoC specific ASV driver probes, reads group ID from hardware register,
> >    calls register_asv_member() with appropriate DVFS table for detected
> >    group.
> > 3) Driver using ASV calls asv_init_opp_table() with its struct device and
> >    ASV member name, which causes the ASV code to fill device's operating
> >    point using OPP calls.
> >
> > Now client driver has all the information it needs and the work of ASV
> > subsystem is done. The control flow between drivers would be much simpler
> > and no callbacks would have to be called.
> >
> 
> Architecture stated above seems to be a subset(one possible way of use),
> of the proposed architecture. If someone really have nothing much to do,
> he can adopt the above stated approach using this framework also,
> callbacks are not mandatory.

I believe that kernel design principles are to first start with something
simple and then if a real need for an extension shows up then extend
existing code base with missing features.

> 
> Since we usually have more things to do other than only reading
> fused group value and simply parsing a table index, so in drivers we have to
> implement functions to segregate stuff and different people do it in
> different way. Its an attempt to provide a way to keep structure(functions)
> similar for easy understanding and factoring out of common code.

I fail to see those more things. Could you elaborate a bit about them?

>From what I see, all the potential ASV users need is a set of operating
points (frequency:voltage pairs) appropriate for the SoC we are running
on (i.e. matching our ASV group index). Is there anything more we need to
do for ASV support?

> 
> Moreover, I feels need of callbacks if we have to do something depending
> upon(specific) the user/instance of  ASV member. One thing came
> in my mind was dev_node may be required if we may think of parsing
> ASV table from DT and may be more things in future.

We can always add such things later, if real need shows up. As I said
above, we should rather start with something simple, to avoid
overdesigning things without knowing real use cases that may show
up later.

Best regards,
Tomasz

Re: [RFC v2 1/4] power: asv: Add common ASV support for Samsung SoCs

[Yadwinder Singh Brar]

Hi Tomasz,

Sorry for being late.

On Sun, Dec 15, 2013 at 10:51 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> On Sunday 15 of December 2013 22:30:13 Yadwinder Singh Brar wrote:
> [snip]
>> >> +
>> >> +     return NULL;
>> >> +}
>> >> +
>> >> +unsigned int asv_get_volt(enum asv_type_id target_type,
>> >> +                                             unsigned int target_freq)
>> >
>> > Do you need this function at all? I believe this is all about populating
>> > device's OPP array with frequencies and voltages according to its ASV
>> > level. Users will be able to query for required voltage using standard OPP
>> > calls then, without a need for ASV specific functions like this one.
>> >
>>
>> Yes, I had put a comment in initial version after commit message :
>> "Hopefully asv_get_volt() can go out in future, once all users start using OPP
>> library." , which seems to be missed in this version.
>> I had kept it for the time being in initial version, to keep it
>> usable(for testing) with
>> existing cpufreq drivers, which need to reworked and may take time.
>
> Hmm, at the moment none of cpufreq drivers use ASV, so they need to be
> reworked anyway to use it either by the means of a private get_volt
> function or OPP framework. I agree that OPP may require more work,
> though.
>
> If we decide to keep this function in final version, a comment should be
> added saying that its usage is deprecated in favor of generic OPP helpers.
>

yes.

>>
>> [snip]
>> >> +
>> >> +     for (i = 0; i < asv_info->nr_dvfs_level; i++) {
>> >> +             if (dev_pm_opp_add(dev, dvfs_table[i].freq * 1000,
>> >> +                     dvfs_table[i].volt)) {
>> >> +                     dev_warn(dev, "Failed to add OPP %d\n",
>> >> +                              dvfs_table[i].freq);
>> >
>> > Hmm, shouldn't it be considered a failure instead?
>> >
>>
>> hmm, not really always. Theoretically system with some less(failed to add)
>> levels can work. Moreover I had prefered to keep it only warning, just to
>> keep the behaviour of  asv_init_opp_table() similar to that of its
>> counter part of_init_opp_table().
>
> I'm not quite convinced about it. If dev_pm_opp_add() fails, doesn't it mean
> that something broke seriously in upper layer and we should propagate the
> error down? Especially when looking at opp_add(), the only failure
> conditions I can find are memory allocation errors which mean that the
> system is unlikely to operate correctly anyway.
>

yes, for the time being i had prefered to keep it similar to
of_init_opp_table() behaviour wise.
If required both should be fixed.


>> [snip]
>> >
>> > Hmm, I don't see a point of these three separate callbacks above.
>> >
>> > In general, I'd suggest a different architecture. I'd see this more as:
>> >
>> > 1) Platform code registers static platform device to instantiate SoC ASV
>> >    driver.
>> > 2) SoC specific ASV driver probes, reads group ID from hardware register,
>> >    calls register_asv_member() with appropriate DVFS table for detected
>> >    group.
>> > 3) Driver using ASV calls asv_init_opp_table() with its struct device and
>> >    ASV member name, which causes the ASV code to fill device's operating
>> >    point using OPP calls.
>> >
>> > Now client driver has all the information it needs and the work of ASV
>> > subsystem is done. The control flow between drivers would be much simpler
>> > and no callbacks would have to be called.
>> >
>>
>> Architecture stated above seems to be a subset(one possible way of use),
>> of the proposed architecture. If someone really have nothing much to do,
>> he can adopt the above stated approach using this framework also,
>> callbacks are not mandatory.
>
> I believe that kernel design principles are to first start with something
> simple and then if a real need for an extension shows up then extend
> existing code base with missing features.
>

Sorry, I can't see it complex as with architecture stated above
also we have to implement similar structure in drivers as we are already
doing now individually in each soc driver.

>>
>> Since we usually have more things to do other than only reading
>> fused group value and simply parsing a table index, so in drivers we have to
>> implement functions to segregate stuff and different people do it in
>> different way. Its an attempt to provide a way to keep structure(functions)
>> similar for easy understanding and factoring out of common code.
>
> I fail to see those more things. Could you elaborate a bit about them?

Usually we need to implement functions in drivers clearly demarking following :
1- Reading chip info (which can be done at probe time only once for all).
2- Parse/Calculate(modify) ASV group.
3- Any Group specific one time setting. eg ABB settings.
4- Parsing and modifying table ( implementing Voltage locking, if
required based on locking info bits).


Best Regards,
Yadwinder

Re: [RFC v2 0/4] Add basic support for ASV

[Kevin Hilman]

On Tue, Dec 3, 2013 at 10:00 PM, Sachin Kamat <sachin.kamat@linaro.org> wrote:
> Hi Abhilash,
>
> On 3 December 2013 20:16, Abhilash Kesavan <kesavan.abhilash@gmail.com> wrote:
>> Hi Yadwinder and Sachin,
>
>> CC'ing Doug and Andrew who have also worked on ASV.
>>
>> I tested these patches on a 5250 Chromebook after modifying the
>> cpufreq code and a few other changes for booting the board. The driver
>> is retrieving the ASV fused group correctly. The behavior on an
>> unfused SMDK5250 is also fine.
>> I have a few minor comments on the patches.
>>
>
> Thank you for testing and reviewing the patchset.
> Will incorporate your comments in the next version.

Has there been an updated version of this series posted?    I can't
seem to find one.

Kevin