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

[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

[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

[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

[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

[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

[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

[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

[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

[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

[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