RE: [PATCH 07/16] OMAP3: PM: Adding smartreflex class 3 driver.

Gopinath, Thara
2010-03-05T15:04:06Z

>>-----Original Message-----
>>From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
>>Sent: Wednesday, March 03, 2010 1:07 AM
>>To: Gopinath, Thara
>>Cc: linux-omap@vger.kernel.org; paul@pwsan.com; Menon, Nishanth; Cousson, Benoit; Sripathy,
>>Vishwanath; Sawant, Anand
>>Subject: Re: [PATCH 07/16] OMAP3: PM: Adding smartreflex class 3 driver.
>>
>>Thara Gopinath <thara@ti.com> writes:
>>
>>> This patch adds smartreflex class 3 driver. This driver hooks
>>> up with the generic smartreflex driver smartreflex.c to abstract
>>> out class specific implementations out of the generic driver.
>>>
>>> Signed-off-by: Thara Gopinath <thara@ti.com>
>>
>>I like this abstraction, but I don't like that it still uses OPP IDs.
>>
>>So, before doing this, we need to get rid of OPP IDs from the
>>smartreflex layer and keep OPP ID usage isolated to SRF.
>>
>>It appears the only reason we need OPP IDs in SR is because the Nvalue
>>tables are indexed by OPP ID in sr_enable().

Actually this is not true. If you see my patch14 sr err_minlimit and vp errorgain are opp dependent. In fact for 3630 for VDD1 there are 4 supported OPP's and 4 different values for err_minlimit and errorgain - one for each opp. So we might still need the opp id.
>>
>>One way to fix this is for this SR layer to keep it's own mapping of
>>voltage to nvalue. So instead of taking OPP ID, sr_enable() should
>>take a voltage (or frequency) and use that to look up the nvalue.
>>
>>a couple other minor comments below...

Thank you for these comments and I will fix them in V2

Regards
Thara
>>
>>> ---
>>> arch/arm/mach-omap2/Makefile | 1 +
>>> arch/arm/mach-omap2/smartreflex-class3.c | 49 ++++++++++++++++++++++++++++++
>>> arch/arm/plat-omap/Kconfig | 11 ++++++-
>>> 3 files changed, 60 insertions(+), 1 deletions(-)
>>> create mode 100644 arch/arm/mach-omap2/smartreflex-class3.c
>>>
>>> diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
>>> index f6f901f..cd8ab86 100644
>>> --- a/arch/arm/mach-omap2/Makefile
>>> +++ b/arch/arm/mach-omap2/Makefile
>>> @@ -52,6 +52,7 @@ obj-$(CONFIG_ARCH_OMAP2) += sleep24xx.o
>>> obj-$(CONFIG_ARCH_OMAP3) += pm34xx.o sleep34xx.o cpuidle34xx.o
>>> obj-$(CONFIG_PM_DEBUG) += pm-debug.o
>>> obj-$(CONFIG_OMAP_SMARTREFLEX) += smartreflex.o
>>> +obj-$(CONFIG_OMAP_SMARTREFLEX_CLASS3) += smartreflex-class3.o
>>>
>>> AFLAGS_sleep24xx.o :=-Wa,-march=armv6
>>> AFLAGS_sleep34xx.o :=-Wa,-march=armv7-a
>>> diff --git a/arch/arm/mach-omap2/smartreflex-class3.c b/arch/arm/mach-omap2/smartreflex-class3.c
>>> new file mode 100644
>>> index 0000000..d2e98a5
>>> --- /dev/null
>>> +++ b/arch/arm/mach-omap2/smartreflex-class3.c
>>> @@ -0,0 +1,49 @@
>>> +/*
>>> + * Smart reflex Class 3 specific implementations
>>> + *
>>> + * Copyright (C) 2009 Texas Instruments, Inc.
>>
>>2010
>>
>>> + * Thara Gopinath <thara@ti.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 "smartreflex.h"
>>> +
>>> +static int sr_class3_enable(int id)
>>> +{
>>> + int target_opp_no = 0;
>>
>>insert blank line
>>
>>> + if (id == SR1)
>>> + target_opp_no = get_vdd1_opp();
>>> + else if (id == SR2)
>>> + target_opp_no = get_vdd2_opp();
>>> + if (!target_opp_no) {
>>> + pr_warning("Targetopp not known.Cannot enable SR%d\n", id);
>>> + return false;
>>> + }
>>
>>and here
>>
>>> + return sr_enable(id, target_opp_no);
>>> +}
>>>
>>> +static int sr_class3_disable(int id)
>>> +{
>>> + int target_opp_no = 0;
>>
>>blank line
>>
>>> + if (id == SR1)
>>> + target_opp_no = get_vdd1_opp();
>>> + else if (id == SR2)
>>> + target_opp_no = get_vdd2_opp();
>>> + sr_disable(id);
>>
>>blank line
>>
>>> + return true;
>>> +}
>>> +/* SR class3 structure */
>>> +struct omap_smartreflex_class_data class3_data = {
>>> + .enable = sr_class3_enable,
>>> + .disable = sr_class3_disable,
>>> +};
>>> +
>>> +static int __init sr_class3_init(void)
>>> +{
>>> + omap_sr_register_class(&class3_data);
>>> + return 0;
>>> +}
>>> +late_initcall(sr_class3_init);
>>> diff --git a/arch/arm/plat-omap/Kconfig b/arch/arm/plat-omap/Kconfig
>>> index cef67f3..9d286e6 100644
>>> --- a/arch/arm/plat-omap/Kconfig
>>> +++ b/arch/arm/plat-omap/Kconfig
>>> @@ -54,7 +54,7 @@ config OMAP_DEBUG_LEDS
>>>
>>> config OMAP_SMARTREFLEX
>>> bool "SmartReflex support"
>>> - depends on ARCH_OMAP3 && TWL4030_CORE && PM
>>> + depends on ARCH_OMAP3 && PM
>>> help
>>> Say Y if you want to enable SmartReflex.
>>>
>>> @@ -69,6 +69,15 @@ config OMAP_SMARTREFLEX
>>> compensation for VDD1 and VDD2, user must write 1 to
>>> /sys/power/sr_vddX_autocomp, where X is 1 or 2.
>>>
>>> +config OMAP_SMARTREFLEX_CLASS3
>>> + bool "Class 3 mode of Smartreflex Implementation"
>>> + depends on OMAP_SMARTREFLEX && TWL4030_CORE
>>> + help
>>> + Say Y to enable Class 3 implementation of Smartreflex
>>> +
>>> + Class 3 implementation of Smartreflex employs continuous hardware
>>> + voltage caliberation.
>>> +
>>> config OMAP_SMARTREFLEX_TESTING
>>> bool "Smartreflex testing support"
>>> depends on OMAP_SMARTREFLEX
>>> --
>>> 1.7.0.rc1.33.g07cf0f