"Gopinath, Thara" <thara@xxxxxx> writes: >>>-----Original Message----- >>>From: Kevin Hilman [mailto:khilman@xxxxxxxxxxxxxxxxxxx] >>>Sent: Wednesday, March 03, 2010 12:14 AM >>>To: Gopinath, Thara >>>Cc: linux-omap@xxxxxxxxxxxxxxx; paul@xxxxxxxxx; Menon, Nishanth; Cousson, Benoit; Sripathy, >>>Vishwanath; Sawant, Anand >>>Subject: Re: [PATCH 06/16] OMAP3: PM: Smartreflex class related changes for smartreflex.c >>> >>>Thara Gopinath <thara@xxxxxx> writes: >>> >>>> OMAP3 smartreflex modules are capable of two different classes >>>> of implementaion - >>>> Class-2: Continuous Software Calibration >>>> Class-3: Continuous Hardware Calibration. >>>> OMAP3 along with T2/Gaia supports the Class 3 implementaion. >>>> With a different PMIC it can support Class 2 implementaion also. >>>> >>>> The idea behind this patch is that smartreflex.c should be able >>>> to support both the classes of Smartreflex and the class specific >>>> details for smartreflex should stay out of this file in a separate >>>> class file. >>>> This patch introduces smartreflex class specific hooks in >>>> smartreflex.c. This patch only takes care of smartreflex enable >>>> disable hooks which differ between Class 2 and Class 3. There >>>> are some register setting changes between both the classes which >>>> will be taken care of in a later patch. >>>> This will form the base for adding class specific >>>> drivers in later patches. >>>> >>>> Signed-off-by: Thara Gopinath <thara@xxxxxx> >>> >>>Minor nit: the name change for omap_smartreflex_[enable|disable] is >>>unrelated to this change and should be a separate patch to go early in >>>the series. > > Hi Kevin, > > I agree the name change need not be part of this patch. But is it necessary for it to be separated out. I am asking this because when I send out the review comments fixed patch set I can send them out as V2 . But if I split this change into a separate patch the patch set will contain 17 patches instead of 16 patches and will have to be submitted as a new patch set not as V2. Do let me know your thoughts on this. Yes, I think it should be split out. It's common for a V2 series to contain more or less patches than the original. In the 'PATCH 0/x' just summarize those differences. Most maintainers (including myself) prefer that when updates are (re)sent, the whole series is (re)sent as well, even if there were changes in only some of the patches. It is easier to track with automated tools (like patchwork) when all the patches are together in a series. Thanks, Kevin > >>> >>>Kevin >>> >>>> --- >>>> arch/arm/mach-omap2/pm34xx.c | 8 +- >>>> arch/arm/mach-omap2/smartreflex.c | 251 ++++++++++++++++++++---------------- >>>> arch/arm/mach-omap2/smartreflex.h | 48 ++++++-- >>>> 3 files changed, 182 insertions(+), 125 deletions(-) >>>> >>>> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c >>>> index cf9ca23..ece5195 100644 >>>> --- a/arch/arm/mach-omap2/pm34xx.c >>>> +++ b/arch/arm/mach-omap2/pm34xx.c >>>> @@ -430,9 +430,9 @@ void omap_sram_idle(void) >>>> * Only needed if we are going to enter retention or off. >>>> */ >>>> if (mpu_next_state <= PWRDM_POWER_RET) >>>> - disable_smartreflex(SR1); >>>> + omap_smartreflex_disable(SR1); >>>> if (core_next_state <= PWRDM_POWER_RET) >>>> - disable_smartreflex(SR2); >>>> + omap_smartreflex_disable(SR2); >>>> >>>> /* CORE */ >>>> if (core_next_state < PWRDM_POWER_ON) { >>>> @@ -531,9 +531,9 @@ void omap_sram_idle(void) >>>> * retention or off >>>> */ >>>> if (mpu_next_state <= PWRDM_POWER_RET) >>>> - enable_smartreflex(SR1); >>>> + omap_smartreflex_enable(SR1); >>>> if (core_next_state <= PWRDM_POWER_RET) >>>> - enable_smartreflex(SR2); >>>> + omap_smartreflex_enable(SR2); >>>> >>>> /* PER */ >>>> if (per_next_state < PWRDM_POWER_ON) { >>>> diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-omap2/smartreflex.c >>>> index c00925d..ba9f899 100644 >>>> --- a/arch/arm/mach-omap2/smartreflex.c >>>> +++ b/arch/arm/mach-omap2/smartreflex.c >>>> @@ -55,6 +55,7 @@ struct omap_sr { >>>> >>>> /* sr_list contains all the instances of smartreflex module */ >>>> static LIST_HEAD(sr_list); >>>> +static struct omap_smartreflex_class_data *sr_class; >>>> >>>> #define SR_REGADDR(offs) (sr->srbase_addr + offset) >>>> >>>> @@ -388,14 +389,86 @@ static int sr_reset_voltage(int srid) >>>> return 0; >>>> } >>>> >>>> -static int sr_enable(struct omap_sr *sr, u32 target_opp_no) >>>> +static void sr_start_vddautocomap(int srid) >>>> +{ >>>> + struct omap_sr *sr = _sr_lookup(srid); >>>> + >>>> + if (!sr) { >>>> + pr_warning("omap_sr struct corresponding to SR%d not found\n", >>>> + srid); >>>> + return; >>>> + } >>>> + >>>> + if (!sr_class || !(sr_class->enable)) { >>>> + pr_warning("smartreflex class driver not registered\n"); >>>> + return; >>>> + } >>>> + >>>> + if (sr->is_sr_reset == 1) { >>>> + sr_clk_enable(sr); >>>> + sr_configure(sr); >>>> + } >>>> + >>>> + sr->is_autocomp_active = 1; >>>> + if (!sr_class->enable(srid)) { >>>> + sr->is_autocomp_active = 0; >>>> + if (sr->is_sr_reset == 1) >>>> + sr_clk_disable(sr); >>>> + } >>>> +} >>>> + >>>> +static void sr_stop_vddautocomap(int srid) >>>> +{ >>>> + struct omap_sr *sr = _sr_lookup(srid); >>>> + >>>> + if (!sr) { >>>> + pr_warning("omap_sr struct corresponding to SR%d not found\n", >>>> + srid); >>>> + return; >>>> + } >>>> + if (!sr_class || !(sr_class->disable)) { >>>> + pr_warning("smartreflex class driver not registered\n"); >>>> + return; >>>> + } >>>> + >>>> + if (sr->is_autocomp_active == 1) { >>>> + sr_class->disable(srid); >>>> + sr_clk_disable(sr); >>>> + sr->is_autocomp_active = 0; >>>> + /* Reset the volatage for current OPP */ >>>> + sr_reset_voltage(srid); >>>> + } >>>> + >>>> +} >>>> + >>>> +/* Public Functions */ >>>> + >>>> +/** >>>> + * sr_enable : Enables the smartreflex module. >>>> + * @srid - The id of the sr module to be enabled. >>>> + * @target_opp_no - The OPP at which the Voltage domain associated with >>>> + * the smartreflex module is operating at. This is required only to program >>>> + * the correct Ntarget value. >>>> + * >>>> + * This API is to be called from the smartreflex class driver to >>>> + * enable a smartreflex module. Returns true on success.Returns false if the >>>> + * target opp id passed is wrong or if ntarget value is wrong. >>>> + */ >>>> +int sr_enable(int srid, u32 target_opp_no) >>>> { >>>> u32 nvalue_reciprocal, v; >>>> + struct omap_sr *sr = _sr_lookup(srid); >>>> struct omap_opp *opp; >>>> struct omap_smartreflex_data *pdata = sr->pdev->dev.platform_data; >>>> int uvdc; >>>> char vsel; >>>> >>>> + if (!sr) { >>>> + pr_warning("omap_sr struct corresponding to SR%d not found\n", >>>> + srid); >>>> + return false; >>>> + } >>>> + >>>> if (sr->srid == SR1) { >>>> opp = opp_find_by_opp_id(OPP_MPU, target_opp_no); >>>> if (!opp) >>>> @@ -477,8 +550,16 @@ static int sr_enable(struct omap_sr *sr, u32 target_opp_no) >>>> return true; >>>> } >>>> >>>> -static void sr_disable(struct omap_sr *sr) >>>> +/** >>>> + * sr_disable : Disables the smartreflex module. >>>> + * @srid - The id of the sr module to be disabled. >>>> + * >>>> + * This API is to be called from the smartreflex class driver to >>>> + * disable a smartreflex module. >>>> + */ >>>> +void sr_disable(int srid) >>>> { >>>> + struct omap_sr *sr = _sr_lookup(srid); >>>> u32 i = 0; >>>> >>>> sr->is_sr_reset = 1; >>>> @@ -518,9 +599,19 @@ static void sr_disable(struct omap_sr *sr) >>>> } >>>> } >>>> >>>> - >>>> -void sr_start_vddautocomap(int srid, u32 target_opp_no) >>>> +/** >>>> + * omap_smartreflex_enable : API to enable SR clocks and to call into the >>>> + * registered smartreflex class enable API. >>>> + * @srid - The id of the sr module to be enabled. >>>> + * >>>> + * This API is to be called from the kernel in order to enable >>>> + * a particular smartreflex module. This API will do the initial >>>> + * configurations to turn on the smartreflex module and in turn call >>>> + * into the registered smartreflex class enable API. >>>> + */ >>>> +void omap_smartreflex_enable(int srid) >>>> { >>>> + u32 target_opp_no = 0; >>>> struct omap_sr *sr = _sr_lookup(srid); >>>> >>>> if (!sr) { >>>> @@ -528,52 +619,8 @@ void sr_start_vddautocomap(int srid, u32 target_opp_no) >>>> srid); >>>> return; >>>> } >>>> - >>>> - if (sr->is_sr_reset == 1) { >>>> - sr_clk_enable(sr); >>>> - sr_configure(sr); >>>> - } >>>> - >>>> - sr->is_autocomp_active = 1; >>>> - if (!sr_enable(sr, target_opp_no)) { >>>> - sr->is_autocomp_active = 0; >>>> - if (sr->is_sr_reset == 1) >>>> - sr_clk_disable(sr); >>>> - } >>>> -} >>>> -EXPORT_SYMBOL(sr_start_vddautocomap); >>>> - >>>> -int sr_stop_vddautocomap(int srid) >>>> -{ >>>> - struct omap_sr *sr = _sr_lookup(srid); >>>> - >>>> - if (!sr) { >>>> - pr_warning("omap_sr struct corresponding to SR%d not found\n", >>>> - srid); >>>> - return false; >>>> - } >>>> - >>>> - if (sr->is_autocomp_active == 1) { >>>> - sr_disable(sr); >>>> - sr_clk_disable(sr); >>>> - sr->is_autocomp_active = 0; >>>> - /* Reset the volatage for current OPP */ >>>> - sr_reset_voltage(srid); >>>> - return true; >>>> - } else >>>> - return false; >>>> - >>>> -} >>>> -EXPORT_SYMBOL(sr_stop_vddautocomap); >>>> - >>>> -void enable_smartreflex(int srid) >>>> -{ >>>> - u32 target_opp_no = 0; >>>> - struct omap_sr *sr = _sr_lookup(srid); >>>> - >>>> - if (!sr) { >>>> - pr_warning("omap_sr struct corresponding to SR%d not found\n", >>>> - srid); >>>> + if (!sr_class || !(sr_class->enable)) { >>>> + pr_warning("smartreflex class driver not registered\n"); >>>> return; >>>> } >>>> >>>> @@ -581,26 +628,24 @@ void enable_smartreflex(int srid) >>>> if (sr->is_sr_reset == 1) { >>>> /* Enable SR clks */ >>>> sr_clk_enable(sr); >>>> - >>>> - if (srid == SR1) >>>> - target_opp_no = get_vdd1_opp(); >>>> - else if (srid == SR2) >>>> - target_opp_no = get_vdd2_opp(); >>>> - >>>> - if (!target_opp_no) { >>>> - pr_info("Current OPP unknown \ >>>> - Cannot configure SR\n"); >>>> - } >>>> - >>>> sr_configure(sr); >>>> >>>> - if (!sr_enable(sr, target_opp_no)) >>>> + if (!sr_class->enable(srid)) >>>> sr_clk_disable(sr); >>>> } >>>> } >>>> } >>>> >>>> -void disable_smartreflex(int srid) >>>> +/** >>>> + * omap_smartreflex_disable : API to disable SR clocks and to call into the >>>> + * registered smartreflex class disable API. >>>> + * @srid - The id of the sr module to be disabled. >>>> + * >>>> + * This API is to be called from the kernel in order to disable >>>> + * a particular smartreflex module. This API will in turn call >>>> + * into the registered smartreflex class disable API. >>>> + */ >>>> +void omap_smartreflex_disable(int srid) >>>> { >>>> u32 i = 0; >>>> struct omap_sr *sr = _sr_lookup(srid); >>>> @@ -610,54 +655,43 @@ void disable_smartreflex(int srid) >>>> srid); >>>> return; >>>> } >>>> + if (!sr_class || !(sr_class->disable)) { >>>> + pr_warning("smartreflex class driver not registered\n"); >>>> + return; >>>> + } >>>> >>>> if (sr->is_autocomp_active == 1) { >>>> if (sr->is_sr_reset == 0) { >>>> - >>>> - sr->is_sr_reset = 1; >>>> - /* SRCONFIG - disable SR */ >>>> - sr_modify_reg(sr, SRCONFIG, SRCONFIG_SRENABLE, >>>> - ~SRCONFIG_SRENABLE); >>>> - >>>> + sr_class->disable(srid); >>>> /* Disable SR clk */ >>>> sr_clk_disable(sr); >>>> - if (sr->srid == SR1) { >>>> - /* Wait for VP idle before disabling VP */ >>>> - while ((!prm_read_mod_reg(OMAP3430_GR_MOD, >>>> - OMAP3_PRM_VP1_STATUS_OFFSET)) >>>> - && i++ < MAX_TRIES) >>>> - udelay(1); >>>> - >>>> - if (i >= MAX_TRIES) >>>> - pr_warning("VP1 not idle, still going \ >>>> - ahead with VP1 disable\n"); >>>> - >>>> - /* Disable VP1 */ >>>> - prm_clear_mod_reg_bits(PRM_VP1_CONFIG_VPENABLE, >>>> - OMAP3430_GR_MOD, >>>> - OMAP3_PRM_VP1_CONFIG_OFFSET); >>>> - } else if (sr->srid == SR2) { >>>> - /* Wait for VP idle before disabling VP */ >>>> - while ((!prm_read_mod_reg(OMAP3430_GR_MOD, >>>> - OMAP3_PRM_VP2_STATUS_OFFSET)) >>>> - && i++ < MAX_TRIES) >>>> - udelay(1); >>>> - >>>> - if (i >= MAX_TRIES) >>>> - pr_warning("VP2 not idle, still going \ >>>> - ahead with VP2 disable\n"); >>>> - >>>> - /* Disable VP2 */ >>>> - prm_clear_mod_reg_bits(PRM_VP2_CONFIG_VPENABLE, >>>> - OMAP3430_GR_MOD, >>>> - OMAP3_PRM_VP2_CONFIG_OFFSET); >>>> - } >>>> /* Reset the volatage for current OPP */ >>>> sr_reset_voltage(srid); >>>> } >>>> } >>>> } >>>> >>>> +/** >>>> + * omap_sr_register_class : API to register a smartreflex class parameters. >>>> + * @class_data - The structure containing various sr class specific data. >>>> + * >>>> + * This API is to be called by the smartreflex class driver to register itself >>>> + * with the smartreflex driver during init. >>>> + */ >>>> +void omap_sr_register_class(struct omap_smartreflex_class_data *class_data) >>>> +{ >>>> + if (!class_data) { >>>> + pr_warning("Smartreflex class data passed is NULL\n"); >>>> + return; >>>> + } >>>> + >>>> + if (sr_class) { >>>> + pr_warning("Smartreflex class driver already registered\n"); >>>> + return; >>>> + } >>>> + sr_class = class_data; >>>> +} >>>> + >>>> /* Voltage Scaling using SR VCBYPASS */ >>>> int sr_voltagescale_vcbypass(u32 target_opp, u32 current_opp, >>>> u8 target_vsel, u8 current_vsel) >>>> @@ -730,9 +764,9 @@ int sr_voltagescale_vcbypass(u32 target_opp, u32 current_opp, >>>> >>>> if (sr_status) { >>>> if (vdd == VDD1_OPP) >>>> - sr_start_vddautocomap(SR1, target_opp_no); >>>> + sr_start_vddautocomap(SR1); >>>> else if (vdd == VDD2_OPP) >>>> - sr_start_vddautocomap(SR2, target_opp_no); >>>> + sr_start_vddautocomap(SR2); >>>> } >>>> >>>> return 0; >>>> @@ -762,17 +796,10 @@ static int omap_sr_autocomp_store(void *data, u64 val) >>>> sr_info->srid); >>>> return 0; >>>> } >>>> - if (val == 0) { >>>> + if (val == 0) >>>> sr_stop_vddautocomap(sr_info->srid); >>>> - } else { >>>> - u32 current_opp; >>>> - >>>> - if (sr_info->srid == SR1) >>>> - current_opp = get_vdd1_opp(); >>>> - else >>>> - current_opp = get_vdd2_opp(); >>>> - sr_start_vddautocomap(sr_info->srid, current_opp); >>>> - } >>>> + else >>>> + sr_start_vddautocomap(sr_info->srid); >>>> return 0; >>>> } >>>> >>>> diff --git a/arch/arm/mach-omap2/smartreflex.h b/arch/arm/mach-omap2/smartreflex.h >>>> index 572cdca..a59a073 100644 >>>> --- a/arch/arm/mach-omap2/smartreflex.h >>>> +++ b/arch/arm/mach-omap2/smartreflex.h >>>> @@ -243,12 +243,27 @@ extern u32 current_vdd2_opp; >>>> #define SR_TESTING_NVALUES 0 >>>> #endif >>>> >>>> -/* >>>> - * Smartreflex module enable/disable interface. >>>> - * NOTE: if smartreflex is not enabled from sysfs, these functions will not >>>> - * do anything. >>>> - */ >>>> #ifdef CONFIG_OMAP_SMARTREFLEX >>>> +/** >>>> + * omap_smartreflex_class_data : Structure to be populated by >>>> + * Smartreflex class driver with corresponding class enable disable API's >>>> + * >>>> + * @enable - API to enable a particular class smaartreflex. >>>> + * @disable - API to disable a particular class smartreflex. >>>> + * @notify - API to notify the class driver about an event in SR. Not needed >>>> + * for class3. >>>> + * @notify_flags - specify the events to be notified to the class driver >>>> + * @class_type - specify which smartreflex class. Can be used by the SR driver >>>> + * to tkae any class based decisions. >>>> + */ >>>> +struct omap_smartreflex_class_data { >>>> + int (*enable)(int sr_id); >>>> + int (*disable)(int sr_id); >>>> + int (*notify)(int sr_id, u32 status); >>>> + u8 notify_flags; >>>> + u8 class_type; >>>> +}; >>>> + >>>> /* >>>> * omap_smartreflex_data - Smartreflex platform data >>>> * >>>> @@ -274,11 +289,26 @@ struct omap_smartreflex_data { >>>> int (*device_idle)(struct platform_device *pdev); >>>> }; >>>> >>>> -void enable_smartreflex(int srid); >>>> -void disable_smartreflex(int srid); >>>> +/* >>>> + * Smartreflex module enable/disable interface. >>>> + * NOTE: if smartreflex is not enabled from sysfs, these functions will not >>>> + * do anything. >>>> + */ >>>> +void omap_smartreflex_enable(int srid); >>>> +void omap_smartreflex_disable(int srid); >>>> + >>>> +/** >>>> + * Smartreflex driver hooks to be called from Smartreflex class driver >>>> + */ >>>> +int sr_enable(int srid, u32 target_opp_no); >>>> +void sr_disable(int srid); >>>> + >>>> +/** >>>> + * API to register the smartreflex class driver with the smartreflex driver >>>> + */ >>>> +void omap_sr_register_class(struct omap_smartreflex_class_data *class_data); >>>> + >>>> int sr_voltagescale_vcbypass(u32 t_opp, u32 c_opp, u8 t_vsel, u8 c_vsel); >>>> -void sr_start_vddautocomap(int srid, u32 target_opp_no); >>>> -int sr_stop_vddautocomap(int srid); >>>> #else >>>> static inline void enable_smartreflex(int srid) {} >>>> static inline void disable_smartreflex(int srid) {} >>>> -- >>>> 1.7.0.rc1.33.g07cf0f -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
