From: "Turquette, Mike" <mturquette@ti.com>
To: "Gulati, Shweta" <shweta.gulati@ti.com>
Cc: linux-omap@vger.kernel.org
Subject: Re: [PATCH 4/4] OMAP: Voltage: Adaptive Body-Bias handlers
Date: Tue, 1 Mar 2011 11:30:27 -0600 [thread overview]
Message-ID: <AANLkTimY1Yx4F37xY0b8OxaK0n=yR7s2edHWQQco=uRi@mail.gmail.com> (raw)
In-Reply-To: <AANLkTimMU0fktzafBHFyL0vMYh0P4kVaQs4yLq624Fiq@mail.gmail.com>
On Mon, Feb 28, 2011 at 3:35 AM, Gulati, Shweta <shweta.gulati@ti.com> wrote:
> Hi,
>
> On Tue, Feb 22, 2011 at 1:17 AM, Turquette, Mike <mturquette@ti.com> wrote:
>> On Mon, Feb 21, 2011 at 4:31 AM, Gulati, Shweta <shweta.gulati@ti.com> wrote:
>>> Hi,
>>>
>>> On Fri, Feb 18, 2011 at 2:50 PM, Mike Turquette <mturquette@ti.com> wrote:
>>>> Introduce voltage transition notification handlers for Adaptive Body-Bias
>>>> LDOs. There is an ABB LDO for VDD_MPU on OMAP3630 and an ABB_LDO on VDD_MPU
>>>> and VDD_IVA on OMAP4430.
>>>>
>>>> All of these LDOs are handled similary. Initial configuration is to enable
>>>> the possibility of going into Forward Body-Bias (which boosts voltage at
>>>> high OPPs). This feature was designed for weak silicon, and eFuse values
>>>> exist to control whether or not this feature should be turned on. However
>>>> recommendations from hardware folks always say to leave it on regardless of
>>>> eFuse values, so we don't bother checking them. For all other OPPs the LDO
>>>> is in bypass and will follow the voltage of it's corresponding VDD_xxx.
>>>> Reverse Body-Bias exists but we never use this in practice (for saving power
>>>> on strongly characterised silicon).
>>> Would RBB be never enabled in future for any of the OPPs or any platforms
>>> that way SLOW_OPP should also be added in OPP types
>>
>> Will do this in next version. RBB is a possibility.
>>
>>>> Upon a DVFS transition the notifiers handle the sequencing of voltage
>>>> scaling and ABB LDO transitions. When moving to a higher OPP that needs
>>>> FBB, raise voltage first and then enable FBB. When moving down to an OPP
>>>> that bypasses ABB, first bypass the LDO then lower voltage.
>>>>
>>>> Signed-off-by: Mike Turquette <mturquette@ti.com>
>>>> ---
>>>> arch/arm/mach-omap2/voltage.c | 379 ++++++++++++++++++++++++++++-
>>>> arch/arm/plat-omap/include/plat/voltage.h | 6 +-
>>>> 2 files changed, 370 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-omap2/voltage.c
>>>> index 6ede092..644a45f 100644
>>>> --- a/arch/arm/mach-omap2/voltage.c
>>>> +++ b/arch/arm/mach-omap2/voltage.c
>>>> @@ -43,6 +43,16 @@
>>>> #define FAST_OPP 0x1
>>>> #define NOMINAL_OPP 0x0
>>>>
>>>> +/* prototypes used by ABB function pointers */
>>>> +static int omap_abb_notify_voltage(struct notifier_block *nb,
>>>> + unsigned long val, void *data);
>>>> +
>>>> +static int omap3_abb_configure(struct omap_abb_info *abb);
>>>> +static int omap3_abb_set_opp(struct omap_abb_info *abb, int opp_type);
>>>> +
>>>> +static int omap4_abb_configure(struct omap_abb_info *abb);
>>>> +static int omap4_abb_set_opp(struct omap_abb_info *abb, int opp_type);
>>>> +
>>>> static struct omap_vdd_info *vdd_info;
>>>> /*
>>>> * Number of scalable voltage domains.
>>>> @@ -70,9 +80,11 @@ static struct omap_vdd_info omap3_vdd_info[] = {
>>>> = OMAP3_PRM_IRQSTATUS_MPU_OFFSET,
>>>> .done_st_shift = OMAP3630_ABB_LDO_TRANXDONE_ST_SHIFT,
>>>> .done_st_mask = OMAP3630_ABB_LDO_TRANXDONE_ST_MASK,
>>>> - .configure = NULL,
>>>> - .nb_handler = NULL,
>>>> - .set_opp = NULL,
>>>> + .nb = {
>>>> + .notifier_call = omap_abb_notify_voltage,
>>>> + },
>>>> + .configure = omap3_abb_configure,
>>>> + .set_opp = omap3_abb_set_opp,
>>>> },
>>>> },
>>>> {
>>>> @@ -113,9 +125,11 @@ static struct omap_vdd_info omap4_vdd_info[] = {
>>>> = OMAP4_PRM_IRQSTATUS_MPU_2_OFFSET,
>>>> .done_st_shift = OMAP4430_ABB_MPU_DONE_ST_SHIFT,
>>>> .done_st_mask = OMAP4430_ABB_MPU_DONE_ST_MASK,
>>>> - .configure = NULL,
>>>> - .nb_handler = NULL,
>>>> - .set_opp = NULL,
>>>> + .nb = {
>>>> + .notifier_call = omap_abb_notify_voltage,
>>>> + },
>>>> + .configure = omap4_abb_configure,
>>>> + .set_opp = omap4_abb_set_opp,
>>>> },
>>>> },
>>>> {
>>>> @@ -137,9 +151,11 @@ static struct omap_vdd_info omap4_vdd_info[] = {
>>>> = OMAP4_PRM_IRQSTATUS_MPU_OFFSET,
>>>> .done_st_shift = OMAP4430_ABB_IVA_DONE_ST_SHIFT,
>>>> .done_st_mask = OMAP4430_ABB_IVA_DONE_ST_MASK,
>>>> - .configure = NULL,
>>>> - .nb_handler = NULL,
>>>> - .set_opp = NULL,
>>>> + .nb = {
>>>> + .notifier_call = omap_abb_notify_voltage,
>>>> + },
>>>> + .configure = omap4_abb_configure,
>>>> + .set_opp = omap4_abb_set_opp,
>>>> },
>>>> },
>>>> {
>>>> @@ -194,7 +210,7 @@ static struct omap_volt_data omap36xx_vddmpu_volt_data[] = {
>>>> NOMINAL_OPP),
>>>> VOLT_DATA_DEFINE(OMAP3630_VDD_MPU_OPP100_UV,
>>>> OMAP3630_CONTROL_FUSE_OPP100_VDD1, 0xf9, 0x16,
>>>> - NOMINAL_OPP),
>>>> + FAST_OPP),
>>>> VOLT_DATA_DEFINE(OMAP3630_VDD_MPU_OPP120_UV,
>>>> OMAP3630_CONTROL_FUSE_OPP120_VDD1, 0xfa, 0x23,
>>>> NOMINAL_OPP),
>>>> @@ -493,6 +509,74 @@ static void __init vdd_debugfs_init(struct omap_vdd_info *vdd)
>>>> &nom_volt_debug_fops);
>>>> }
>>>>
> What about OMAP4, there volt_data should also be updated.
> One think more, for IVA ABB should not be enabled by default rather
> there should be Kconfig
> option to Enable/Disable it.
You caught a bug, but not the one you think ;-)
The NOMINAL_OPP/FAST_OPP definitions are done in patch 3 of this
series (including OMAP4). However to test that this was done
correctly on a scope I set OPP120 to use FAST_OPP since the higher
OPPs were disabled. This line is a bug and I'll remove in v2.
>>>> +/* voltage transition notification handlers */
>>>> +
>>>> +/**
>>>> + * omap_abb_notify_voltage - voltage change notifier handler for ABB
>>>> + * @nb : notifier block
>>>> + * @val : VOLTSCALE_PRECHANGE or VOLTSCALE_POSTCHANGE
>>>> + * @data : struct omap_volt_change_info for a given voltage domain
>>>> + *
>>>> + * Sets ABB LDO to either bypass or Forward Body-Bias whenever a voltage
>>>> + * change notification is generated. Voltages marked as FAST will result in
>>>> + * FBB operation of ABB LDO and voltages marked as NOMINAL will bypass the
>>>> + * LDO. Does not handle Reverse Body-Bias since there is not benefit for it
>>>> + * on any existing silicon. Returns 0 upon success, negative error code
>>>> + * otherwise.
>>>> + */
>>>> +static int omap_abb_notify_voltage(struct notifier_block *nb,
>>>> + unsigned long val, void *data)
>>>> +{
>>>> + struct omap_volt_change_info *v_info;
>>>> + struct omap_vdd_info *vdd;
>>>> + struct omap_volt_data *curr_volt_data, *target_volt_data;
>>>> + int ret = 0;
>>>> +
>>>> + if (!nb || IS_ERR(nb) || !data || IS_ERR(data)) {
>>>> + pr_warning("%s: invalid data specified\n", __func__);
>>>> + ret = -EINVAL;
>>>> + goto out;
>>>> + }
>>>> +
>>>> + v_info = (struct omap_volt_change_info *)data;
>>>> + vdd = v_info->vdd;
>>>> +
>>>> + /* get the voltdata structures for the current & target voltage */
>>>> + target_volt_data = omap_voltage_get_voltdata(&vdd->voltdm,
>>>> + v_info->target_volt);
>>>> + curr_volt_data = omap_voltage_get_voltdata(&vdd->voltdm,
>>>> + v_info->curr_volt);
>>>> +
>>>> + /* nothing to do here */
>>>> + if (target_volt_data->abb_opp == curr_volt_data->abb_opp)
>>>> + goto out;
>>>> +
>>>> + /*
>>>> + * When the VDD drops from a voltage requiring the ABB LDO to be in
>>>> + * FBB mode to a voltage requiring bypass mode, we must bypass the LDO
>>>> + * before the voltage transition.
>>>> + */
>>>> + if (val == VOLTSCALE_PRECHANGE &&
>>>> + target_volt_data->abb_opp == NOMINAL_OPP) {
>>>> + ret = vdd->abb.set_opp(&vdd->abb, NOMINAL_OPP);
> what I meant is, at this place you can write into OPP_SEL bits of
> SETUP register rather than passing argument of opp_type.
This defeats the point of abstraction! In my previous implementation
of this for Android I did directly write into the registers here.
However I only supported OMAP4 and when it came time to add OMAP3 the
code had yet another ugly cpu_is_omapxxxx() check.
The goal is to completely abstract away the version-specific bits from
this generic notification handler which we rely on the function
pointer to do for us. I'll group the register addresses/offsets
together in the ABB struct, but I won't be changing the set_opp
function signature in v2.
>> +
>>>> + /*
>>>> + * When moving from a voltage requiring the ABB LDO to be bypassed to
>>>> + * a voltage requiring FBB mode, we must change the LDO operation
>>>> + * after the voltage transition.
>>>> + */
>>>> + } else if (val == VOLTSCALE_POSTCHANGE &&
>>>> + target_volt_data->abb_opp == FAST_OPP) {
>>>> + ret = vdd->abb.set_opp(&vdd->abb, FAST_OPP);
>>>> +
> Same
Same
>>>> + /* invalid combination, bail out */
>>>> + } else
>>>> + ret = -EINVAL;
>>>> +
>>>> +out:
>>>> + return ret;
>>>> +}
>>>> +
>>>> /* Voltage scale and accessory APIs */
>>>> static int _pre_volt_scale(struct omap_vdd_info *vdd,
>>>> unsigned long target_volt, u8 *target_vsel, u8 *current_vsel)
>>>> @@ -717,6 +801,142 @@ static int vp_forceupdate_scale_voltage(struct omap_vdd_info *vdd,
>>>> return 0;
>>>> }
>>>>
>>>> +/**
>>>> + * omap3_abb_set_opp - program ABB LDO upon a voltage transition
>>>> + *
>>>> + * @abb : ABB instance being programmed
>>>> + * @opp_type : flag for NOMINAL or FAST OPP
>>>> + */
>>>> +static int omap3_abb_set_opp(struct omap_abb_info *abb, int opp_type)
>>>> +{
>>>> + int ret = 0;
>>>> + int timeout;
>>>> +
>>>> + /* program for NOMINAL OPP or FAST OPP */
>>>> + omap2_prm_rmw_mod_reg_bits(OMAP3630_OPP_SEL_MASK,
>>>> + (opp_type << OMAP3630_OPP_SEL_SHIFT),
>>>> + OMAP3430_GR_MOD, abb->setup_offs);
>>> Rather than passing a parameter 'opp_type' u can set OPP_SEL bit in
>>> SETUP Register in API
>>
>> I'm not sure I follow. The opp_type corresponds to the voltage we're
>> transitioning to, so that data must be passed in to this generic
>> set_opp function.
>>
>>> omap_abb_notify_voltage where if-else are used to check whether ABB
>>> needs to transition or not.
>>
>> The notifier handles this... it will not execute call set_opp function
>> if there is no need to change ABB registers.
> Answered above.
Again, I won't be changing this in v2. I'll abstract the register
offsets away so that OMAP3 and OMAP4 can use the same notification
handler.
>>>> +
>>>> + /* clear ABB ldo interrupt status */
>>>> + omap2_prm_clear_mod_reg_bits(abb->done_st_mask, OCP_MOD,
>>>> + abb->irqstatus_mpu_offs);
>>>> +
>>>> + /* enable ABB LDO OPP change */
>>>> + omap2_prm_set_mod_reg_bits(OMAP3630_OPP_CHANGE_MASK, OMAP3430_GR_MOD,
>>>> + abb->setup_offs);
>>>> +
>>>> + timeout = 0;
>>>> +
>>>> + /* wait until OPP change completes */
>>>> + while ((timeout < ABB_TRANXDONE_TIMEOUT) &&
>>>> + (!(omap2_prm_read_mod_reg(OCP_MOD,
>>>> + abb->irqstatus_mpu_offs) &
>>>> + abb->done_st_mask))) {
>>>> + udelay(1);
>>>> + timeout++;
>>>> + }
>>>> +
>>>> + if (timeout == ABB_TRANXDONE_TIMEOUT)
>>>> + pr_warning("%s: TRANXDONE timed out waiting for OPP change\n",
>>>> + __func__);
>>>> +
>>>> + timeout = 0;
>>>> +
>>>> + /* Clear all pending TRANXDONE interrupts/status */
>>>> + while (timeout < ABB_TRANXDONE_TIMEOUT) {
>>>> + omap2_prm_write_mod_reg((1 << abb->done_st_shift), OCP_MOD,
>>>> + abb->irqstatus_mpu_offs);
>>>> +
>>>> + if (!(omap2_prm_read_mod_reg(OCP_MOD, abb->irqstatus_mpu_offs)
>>>> + & abb->done_st_mask))
>>>> + break;
>>>> +
>>>> + udelay(1);
>>>> + timeout++;
>>>> + }
>>>> +
>>>> + if (timeout == ABB_TRANXDONE_TIMEOUT) {
>>>> + pr_warning("%s: TRANXDONE timed out trying to clear status\n",
>>>> + __func__);
>>>> + ret = -EBUSY;
>>>> + }
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +/**
>>>> + * omap4_abb_set_opp - program ABB LDO upon a voltage transition
>>>> + *
>>>> + * @abb : ABB instance being programmed
>>>> + * @opp_type : flag for NOMINAL or FAST OPP
>>>> + */
>>>> +static int omap4_abb_set_opp(struct omap_abb_info *abb, int opp_type)
>>>> +{
>>>> + int ret = 0;
>>>> + int timeout;
>>>> +
>>>> + /* program for NOMINAL OPP or FAST OPP */
>>>> + omap4_prminst_rmw_inst_reg_bits(OMAP4430_OPP_SEL_MASK,
>>>> + (opp_type << OMAP4430_OPP_SEL_SHIFT),
>>>> + OMAP4430_PRM_PARTITION, OMAP4430_PRM_DEVICE_INST,
>>>> + abb->ctrl_offs);
>>>> +
>>>> + /* clear ABB ldo interrupt status */
>>>> + omap4_prminst_rmw_inst_reg_bits(abb->done_st_mask,
>>>> + (0x0 << abb->done_st_shift), OMAP4430_PRM_PARTITION,
>>>> + OMAP4430_PRM_DEVICE_INST, abb->ctrl_offs);
>>>> +
>>>> + /* enable ABB LDO OPP change */
>>>> + omap4_prminst_rmw_inst_reg_bits(OMAP4430_OPP_CHANGE_MASK,
>>>> + (0x1 << OMAP4430_OPP_CHANGE_SHIFT),
>>>> + OMAP4430_PRM_PARTITION, OMAP4430_PRM_DEVICE_INST,
>>>> + abb->ctrl_offs);
>>>> +
>>> A generic struct can be created which has Masks and shifts of all
>>> register offsets like OMAP4430_OPP_CHANGE_MASK
>>> and OMAP3630_ACTIVE_FBB_SEL_MASK whose objects for OMAP3 and OMAP4 can be used.
>>
>> True. I chose to have the struct differentiate between VDDs, not OMAP
>> chips. Do others have an opinion on this? I guess it comes down to
>> whether we should have OMAP-specific ABB set_opp functions, and that
>> struct data should be per-VDD (like it is in my patch)... OR if we
>> should have a generic ABB set_opp function only that works across all
>> OMAPs and all data is in the struct whether it is VDD- or
>> OMAP-specific. My problem with the latter solution is that it assumes
>> the programming model will not change from OMAP to OMAP...
>>
>>>> + timeout = 0;
>>>> +
>>>> + /* wait until OPP change completes */
>>>> + while ((timeout < ABB_TRANXDONE_TIMEOUT) &&
>>>> + (!(omap4_prminst_read_inst_reg(OMAP4430_PRM_PARTITION,
>>>> + OMAP4430_PRM_DEVICE_INST,
>>>> + abb->irqstatus_mpu_offs)
>>>> + & abb->done_st_mask))) {
>>>> + udelay(1);
>>>> + timeout++;
>>>> + }
>>>> +
>>>> + if (timeout == ABB_TRANXDONE_TIMEOUT)
>>>> + pr_warning("%s: TRANXDONE timed out waiting for OPP change\n",
>>>> + __func__);
>>>> +
>>>> + timeout = 0;
>>>> +
>>>> + /* Clear all pending TRANXDONE interrupts/status */
>>>> + while (timeout < ABB_TRANXDONE_TIMEOUT) {
>>>> + omap4_prminst_write_inst_reg((1 << abb->done_st_shift),
>>>> + OMAP4430_PRM_PARTITION,
>>>> + OMAP4430_PRM_DEVICE_INST,
>>>> + abb->irqstatus_mpu_offs);
>>>> +
>>>> + if (!(omap4_prminst_read_inst_reg(OMAP4430_PRM_PARTITION,
>>>> + OMAP4430_PRM_DEVICE_INST,
>>>> + abb->irqstatus_mpu_offs)
>>>> + & abb->done_st_mask)) {
>>>> + break;
>>>> +
>>>> + udelay(1);
>>>> + timeout++;
>>>> + }
>>>> + }
>>>> +
>>>> + if (timeout == ABB_TRANXDONE_TIMEOUT) {
>>>> + pr_warning("%s: TRANXDONE timed out trying to clear status\n",
>>>> + __func__);
>>>> + ret = -EBUSY;
>>>> + }
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> /* OMAP3 specific voltage init functions */
>>>>
>>>> /*
>>>> @@ -789,6 +1009,64 @@ static void __init omap3_vc_init(struct omap_vdd_info *vdd)
>>>> is_initialized = true;
>>>> }
>>>>
>>>> +/**
>>>> + * omap3_abb_configure - per-VDD configuration of ABB
>>>> + *
>>>> + * @abb : abb instance being initialized
>>>> + */
>>>> +static int omap3_abb_configure(struct omap_abb_info *abb)
>>>> +{
>>>> + int ret = 0;
>>>> + u32 sr2_wt_cnt_val;
>>>> + struct clk *sys_ck;
>>>> + struct omap_vdd_info *vdd;
>>>> +
>>>> + if (!abb || IS_ERR(abb)) {
>>>> + pr_warning("%s: invalid abb\n", __func__);
>>>> + ret = -EINVAL;
>>>> + goto out;
>>>> + }
>>>> +
>>>> + sys_ck = clk_get(NULL, "sys_ck");
>>>> + if (IS_ERR(sys_ck)) {
>>>> + pr_warning("%s: unable to fetch SYS_CK\n", __func__);
>>>> + ret = -ENODEV;
>>>> + goto out;
>>>> + }
>>>> +
>>>> + vdd = container_of(abb, struct omap_vdd_info, abb);
>>>> +
>>>> + /* LDO settling time */
>>>> + sr2_wt_cnt_val = clk_get_rate(sys_ck);
>>>> + sr2_wt_cnt_val = sr2_wt_cnt_val / 1000000 / 16;
>>>> +
>>>> + omap2_prm_rmw_mod_reg_bits(OMAP3630_SR2_WTCNT_VALUE_MASK,
>>>> + (sr2_wt_cnt_val << OMAP3630_SR2_WTCNT_VALUE_SHIFT),
>>>> + OMAP3430_GR_MOD, abb->setup_offs);
>>>> +
>>>> + /* allow FBB operation */
>>>> + omap2_prm_set_mod_reg_bits(OMAP3630_ACTIVE_FBB_SEL_MASK,
>>>> + OMAP3430_GR_MOD, abb->setup_offs);
>>>> +
>>>> + /* do not allow ACTIVE RBB operation */
>>>> + omap2_prm_set_mod_reg_bits(OMAP3630_ACTIVE_RBB_SEL_MASK,
>>>> + OMAP3430_GR_MOD, abb->setup_offs);
>>>> +
>>>> + /* do not allow SLEEP RBB operation */
>>>> + omap2_prm_set_mod_reg_bits(OMAP3630_SLEEP_RBB_SEL_MASK,
>>>> + OMAP3430_GR_MOD, abb->setup_offs);
>>>> +
>>>> + /* enable ABB LDO */
>>>> + omap2_prm_set_mod_reg_bits(OMAP3630_SR2EN_MASK,
>>>> + OMAP3430_GR_MOD, abb->ctrl_offs);
>>> ABB should n't be enabled in this API rather it should be enabled only
>>> when it goes to FBB/RBB mode
>>
>> I disagree. I've verified with hw team that this is perfectly safe as
>> it leaves ABB ldo in bypass and will follow VDD_xxx voltage. No
>> reason to complicate notifier code with some "if
>> (!abb_already_enabled)" check.
>>
>>>> +
>>>> + /* register the notifier handler */
>>>> + omap_voltage_register_notifier(vdd, &abb->nb);
>>>> +
>>>> +out:
>>>> + return ret;
>>>> +}
>>>> +
>>>> /* Sets up all the VDD related info for OMAP3 */
>>>> static int __init omap3_vdd_data_configure(struct omap_vdd_info *vdd)
>>>> {
>>>> @@ -824,6 +1102,9 @@ static int __init omap3_vdd_data_configure(struct omap_vdd_info *vdd)
>>>> vdd->vc_reg.smps_volra_mask = OMAP3430_VOLRA0_MASK;
>>>> vdd->vc_reg.voltsetup_shift = OMAP3430_SETUP_TIME1_SHIFT;
>>>> vdd->vc_reg.voltsetup_mask = OMAP3430_SETUP_TIME1_MASK;
>>>> +
>>>> + /* configure ABB */
>>>> + vdd->abb.configure(&vdd->abb);
>>>> } else if (!strcmp(vdd->voltdm.name, "core")) {
>>>> if (cpu_is_omap3630())
>>>> vdd->volt_data = omap36xx_vddcore_volt_data;
>>>> @@ -975,6 +1256,73 @@ static void __init omap4_vc_init(struct omap_vdd_info *vdd)
>>>> is_initialized = true;
>>>> }
>>>>
>>> To remove code repetition rather than having 2 APIs for abb_configure,
>>> I believe 1 API which has generic register offsets struct objects for
>>> all platforms
>>> (OMAP3 and OMAP4 for now) can be used.
>>> This would be scalable for OMAP5+ also.
>>
>> This is the same as what I've stated above for the ABB set_opp
>> function. We can't be sure what the programming model will be for
>> future OMAPs, so should we really do it as your suggest? It is
>> possible that we retain the same set_opp function pointer, and only
>> implemenet it once since it can be used by both OMAP3 and OMAP4 using
>> more verbose struct data. OMAP5 might have a different programming
>> model altogether, so we will need to retain the set_opp function
>> pointer just to be safe. Thoughts on this?
> I have checked OMAP5 ABB Transition Sequences, they are same in sequence
> of setting/clearing registers bits, so I think a structure of
> registers offsets and masks generic
> across VDDs and OMAPs should be used and 1 API should be there to
> remove code repetition
OK, thanks for checking on the OMAP5 programming model. I'll add
register bits to the abb struct and consolidate the notification
handler into a single generic function (will keep "omap3" in the
function name).
Regards,
Mike
>>>
>>>> +/**
>>>> + * omap4_abb_configure - per-VDD configuration of ABB
>>>> + *
>>>> + * @abb : abb instance being initialized
>>>> + */
>>>> +static int omap4_abb_configure(struct omap_abb_info *abb)
>>>> +{
>>>> + int ret = 0;
>>>> + u32 sr2_wt_cnt_val;
>>>> + struct clk *sys_ck;
>>>> + struct omap_vdd_info *vdd;
>>>> +
>>>> + if (!abb || IS_ERR(abb)) {
>>>> + pr_warning("%s: invalid abb\n", __func__);
>>>> + ret = -EINVAL;
>>>> + goto out;
>>>> + }
>>>> +
>>>> + sys_ck = clk_get(NULL, "sys_clkin_ck");
>>>> + if (IS_ERR(sys_ck)) {
>>>> + pr_warning("%s: unable to fetch SYS_CK", __func__);
>>>> + ret = -ENODEV;
>>>> + goto out;
>>>> + }
>>>> +
>>>> + vdd = container_of(abb, struct omap_vdd_info, abb);
>>>> +
>>>> + /* LDO settling time */
>>>> + sr2_wt_cnt_val = clk_get_rate(sys_ck);
>>>> + sr2_wt_cnt_val = sr2_wt_cnt_val / 1000000 / 16;
>>>> +
>>>> + omap4_prminst_rmw_inst_reg_bits(OMAP4430_SR2_WTCNT_VALUE_MASK,
>>>> + (sr2_wt_cnt_val << OMAP4430_SR2_WTCNT_VALUE_SHIFT),
>>>> + OMAP4430_PRM_PARTITION, OMAP4430_PRM_DEVICE_INST,
>>>> + abb->setup_offs);
>>>> +
>>>> + /* allow FBB operation */
>>>> + omap4_prminst_rmw_inst_reg_bits(OMAP4430_ACTIVE_FBB_SEL_MASK,
>>>> + (1 << OMAP4430_ACTIVE_FBB_SEL_SHIFT),
>>>> + OMAP4430_PRM_PARTITION, OMAP4430_PRM_DEVICE_INST,
>>>> + abb->setup_offs);
>>>> +
>>>> + /* do not allow ACTIVE RBB operation */
>>>> + omap4_prminst_rmw_inst_reg_bits(OMAP4430_ACTIVE_RBB_SEL_MASK,
>>>> + (0 << OMAP4430_ACTIVE_RBB_SEL_SHIFT),
>>>> + OMAP4430_PRM_PARTITION, OMAP4430_PRM_DEVICE_INST,
>>>> + abb->setup_offs);
>>>> +
>>>> + /* do not allow SLEEP RBB operation */
>>>> + omap4_prminst_rmw_inst_reg_bits(OMAP4430_SLEEP_RBB_SEL_MASK,
>>>> + (0 << OMAP4430_SLEEP_RBB_SEL_SHIFT),
>>>> + OMAP4430_PRM_PARTITION, OMAP4430_PRM_DEVICE_INST,
>>>> + abb->setup_offs);
>>>> +
>>>> + /* enable ABB LDO */
>>>> + omap4_prminst_rmw_inst_reg_bits(OMAP4430_SR2EN_MASK,
>>>> + (1 << OMAP4430_SR2EN_SHIFT),
>>>> + OMAP4430_PRM_PARTITION, OMAP4430_PRM_DEVICE_INST,
>>>> + abb->setup_offs);
>>> Same as above.
>>
>> Same as my above comment ;-) This is safe and there is no reason to change it.
>>
>> Regards,
>> Mike
>>
>>>> + /* register the notifier handler */
>>>> + omap_voltage_register_notifier(vdd, &abb->nb);
>>>> +
>>>> +out:
>>>> + return ret;
>>>> +}
>>>> +
>>>> /* Sets up all the VDD related info for OMAP4 */
>>>> static int __init omap4_vdd_data_configure(struct omap_vdd_info *vdd)
>>>> {
>>>> @@ -983,8 +1331,8 @@ static int __init omap4_vdd_data_configure(struct omap_vdd_info *vdd)
>>>>
>>>> if (!vdd->pmic_info) {
>>>> pr_err("%s: PMIC info requried to configure vdd_%s not"
>>>> - "populated.Hence cannot initialize vdd_%s\n",
>>>> - __func__, vdd->voltdm.name, vdd->voltdm.name);
>>>> + "populated.Hence cannot initialize vdd_%s\n",
>>>> + __func__, vdd->voltdm.name, vdd->voltdm.name);
>>>> return -EINVAL;
>>>> }
>>>>
>>>> @@ -1005,6 +1353,9 @@ static int __init omap4_vdd_data_configure(struct omap_vdd_info *vdd)
>>>> vdd->vc_reg.voltsetup_reg =
>>>> OMAP4_PRM_VOLTSETUP_MPU_RET_SLEEP_OFFSET;
>>>> vdd->prm_irqst_reg = OMAP4_PRM_IRQSTATUS_MPU_2_OFFSET;
>>>> +
>>>> + /* configure ABB */
>>>> + vdd->abb.configure(&vdd->abb);
>>>> } else if (!strcmp(vdd->voltdm.name, "core")) {
>>>> vdd->volt_data = omap44xx_vdd_core_volt_data;
>>>> vdd->vp_reg.tranxdone_status =
>>>> @@ -1032,6 +1383,9 @@ static int __init omap4_vdd_data_configure(struct omap_vdd_info *vdd)
>>>> vdd->vc_reg.voltsetup_reg =
>>>> OMAP4_PRM_VOLTSETUP_IVA_RET_SLEEP_OFFSET;
>>>> vdd->prm_irqst_reg = OMAP4_PRM_IRQSTATUS_MPU_OFFSET;
>>>> +
>>>> + /* configure ABB */
>>>> + vdd->abb.configure(&vdd->abb);
>>>> } else {
>>>> pr_warning("%s: vdd_%s does not exisit in OMAP4\n",
>>>> __func__, vdd->voltdm.name);
>>>> @@ -1299,6 +1653,7 @@ int omap_voltage_scale_vdd(struct voltagedomain *voltdm,
>>>>
>>>> /* load notifier chain data */
>>>> v_info.target_volt = target_volt;
>>>> + v_info.curr_volt = vdd->curr_volt;
>>>> v_info.vdd = vdd;
>>>>
>>>> srcu_notifier_call_chain(&vdd->volt_change_notify_chain,
>>>> diff --git a/arch/arm/plat-omap/include/plat/voltage.h b/arch/arm/plat-omap/include/plat/voltage.h
>>>> index af790bf..07997f7 100644
>>>> --- a/arch/arm/plat-omap/include/plat/voltage.h
>>>> +++ b/arch/arm/plat-omap/include/plat/voltage.h
>>>> @@ -235,8 +235,8 @@ struct omap_vdd_dep_info {
>>>> * @irqstatus_mpu_offs : PRM_IRQSTATUS_MPU* register offset
>>>> * @done_st_shift : ABB_vdd_DONE_ST shift
>>>> * @done_st_mask : ABB_vdd_DONE_ST bit mask
>>>> + * @nb : voltage transition notifier block
>>>> * @configure : boot-time configuration
>>>> - * @nb_handler : voltage transition notification handler
>>>> * @set_opp : transition function called from nb_handler
>>>> */
>>>> struct omap_abb_info {
>>>> @@ -245,9 +245,8 @@ struct omap_abb_info {
>>>> u8 irqstatus_mpu_offs;
>>>> u8 done_st_shift;
>>>> u32 done_st_mask;
>>>> + struct notifier_block nb;
>>>> int (*configure) (struct omap_abb_info *abb);
>>>> - int (*nb_handler) (struct notifier_block *nb, unsigned long val,
>>>> - void *data);
>>>> int (*set_opp) (struct omap_abb_info *abb, int opp_type);
>>>> };
>>>>
>>>> @@ -302,6 +301,7 @@ struct omap_vdd_info {
>>>> */
>>>> struct omap_volt_change_info {
>>>> unsigned long target_volt;
>>>> + unsigned long curr_volt;
>>>> struct omap_vdd_info *vdd;
>>>> };
>>>>
>>>> --
>>>> 1.7.1
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>
>>>
>>>
>>>
>>> --
>>> Thanks,
>>> Regards,
>>> Shweta
>>>
>>
>
>
>
> --
> Thanks,
> Regards,
> Shweta
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2011-03-01 17:30 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-02-18 9:20 [PATCH 1/4] OMAP: voltage: pre- and post-transition notifiers Mike Turquette
2011-02-18 9:20 ` [PATCH 2/4] OMAP3630: add missing ABB PRM register definitions Mike Turquette
2011-02-18 9:20 ` [PATCH 3/4] OMAP: Voltage: add ABB structures and data Mike Turquette
2011-03-01 22:05 ` Kevin Hilman
2011-02-18 9:20 ` [PATCH 4/4] OMAP: Voltage: Adaptive Body-Bias handlers Mike Turquette
2011-02-19 0:29 ` David Cohen
2011-02-21 10:31 ` Gulati, Shweta
2011-02-21 19:47 ` Turquette, Mike
2011-02-21 23:01 ` Cousson, Benoit
2011-02-28 9:35 ` Gulati, Shweta
2011-03-01 17:30 ` Turquette, Mike [this message]
2011-03-04 4:53 ` Gulati, Shweta
2011-03-01 22:06 ` [PATCH 1/4] OMAP: voltage: pre- and post-transition notifiers Kevin Hilman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='AANLkTimY1Yx4F37xY0b8OxaK0n=yR7s2edHWQQco=uRi@mail.gmail.com' \
--to=mturquette@ti.com \
--cc=linux-omap@vger.kernel.org \
--cc=shweta.gulati@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.