Smartreflex on 'pm-wip/voltdm' Branch

Smartreflex on 'pm-wip/voltdm' Branch

[Gulati, Shweta]

Kevin,

I am testing Smartreflex on your Branch 'pm-wip/voltdm'. There seems
an issue with reading VP registers.
For OMAP3 and OMAP4 reading debugfs entries shows constant voltage.
Logs are:

OMAP3:
# cat /debug/voltage/vdd_mpu_iva/curr_nominal_volt
1200000
# cat /debug/voltage/vdd_core/curr_nominal_volt
1200000
# echo 1 > /debug/voltage/vdd_mpu_iva/smartreflex/autocomp
[  311.680816] omap_device: smartreflex.0: new worst case activate
latency 0: 91552
# echo 1 > /debug/voltage/vdd_core/smartreflex/autocomp
# cat /debug/voltage/vdd_core/vp/curr_volt
900000
# cat /debug/voltage/vdd_mpu_iva/vp/curr_volt
900000


OMAP4:
 # cat /debug/voltage/vdd_core/curr_nominal_volt
1200000
# cat /debug/voltage/vdd_mpu/curr_nominal_volt
1375000
# cat /debug/voltage/vdd_iva/curr_nominal_volt
1188000
#
# echo 1 > /debug/voltage/vdd_mpu/smartreflex/autocomp
[   61.514038] omap_device: smartreflex.2: new worst case activate
latency 0: 30517
# echo 1 > /debug/voltage/vdd_core/smartreflex/autocomp
# echo 1 > /debug/voltage/vdd_mpu/smartreflex/autocomp
#
# cat /debug/voltage/vdd_iva/curr_nominal_volt
1188000
# cat /debug/voltage/vdd_iva/vp/curr_volt
1200000
#
# echo 1 > /debug/voltage/vdd_iva/smartreflex/autocomp
# cat /debug/voltage/vdd_iva/vp/curr_volt
812500
#
# cat /debug/voltage/vdd_mpu/vp/curr_volt
812500
# cat /debug/voltage/vdd_core/vp/curr_volt
812500
#

This Issue is not reproduced on 'pm-core' branch, seems in the voltage
Layer Clean up Patches somewhere some thing is goofed up.


Thanks,
Regards,
Shweta

RE: Smartreflex on 'pm-wip/voltdm' Branch

[Vishwanath Sripathy]

[-- Attachment #1: Type: text/plain, Size: 2635 bytes --]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Gulati, Shweta
> Sent: Monday, March 28, 2011 4:23 PM
> To: linux-omap@vger.kernel.org; Kevin Hilman
> Subject: Smartreflex on 'pm-wip/voltdm' Branch
>
> Kevin,
>
> I am testing Smartreflex on your Branch 'pm-wip/voltdm'. There seems
> an issue with reading VP registers.
> For OMAP3 and OMAP4 reading debugfs entries shows constant voltage.
> Logs are:
>
> OMAP3:
> # cat /debug/voltage/vdd_mpu_iva/curr_nominal_volt
> 1200000
> # cat /debug/voltage/vdd_core/curr_nominal_volt
> 1200000
> # echo 1 > /debug/voltage/vdd_mpu_iva/smartreflex/autocomp
> [  311.680816] omap_device: smartreflex.0: new worst case activate
> latency 0: 91552
> # echo 1 > /debug/voltage/vdd_core/smartreflex/autocomp
> # cat /debug/voltage/vdd_core/vp/curr_volt
> 900000
> # cat /debug/voltage/vdd_mpu_iva/vp/curr_volt
> 900000
>
>
> OMAP4:
>  # cat /debug/voltage/vdd_core/curr_nominal_volt
> 1200000
> # cat /debug/voltage/vdd_mpu/curr_nominal_volt
> 1375000
> # cat /debug/voltage/vdd_iva/curr_nominal_volt
> 1188000
> #
> # echo 1 > /debug/voltage/vdd_mpu/smartreflex/autocomp
> [   61.514038] omap_device: smartreflex.2: new worst case activate
> latency 0: 30517
> # echo 1 > /debug/voltage/vdd_core/smartreflex/autocomp
> # echo 1 > /debug/voltage/vdd_mpu/smartreflex/autocomp
> #
> # cat /debug/voltage/vdd_iva/curr_nominal_volt
> 1188000
> # cat /debug/voltage/vdd_iva/vp/curr_volt
> 1200000
> #
> # echo 1 > /debug/voltage/vdd_iva/smartreflex/autocomp
> # cat /debug/voltage/vdd_iva/vp/curr_volt
> 812500
> #
> # cat /debug/voltage/vdd_mpu/vp/curr_volt
> 812500
> # cat /debug/voltage/vdd_core/vp/curr_volt
> 812500
> #
>
> This Issue is not reproduced on 'pm-core' branch, seems in the voltage
> Layer Clean up Patches somewhere some thing is goofed up.
I did a quick debug on this and found that the root cause of the issue is
in usage of ffs (because of this, i2c slave address was configured wrongly
in vc).
Basically ffs returns the position of the first (least significant) bit
set in the word and the least significant bit is position 1 where as our
bit operation assumes that least significant position is 0.
I tested the attached patch on OMAP3 and it seems to work fine.

Kevin,
You may want to incorporate this change in your next version if this seems
OK to you.

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

[-- Attachment #2: vc_fix.patch --]
[-- Type: application/octet-stream, Size: 2308 bytes --]

diff --git a/arch/arm/mach-omap2/prm2xxx_3xxx.c b/arch/arm/mach-omap2/prm2xxx_3xxx.c
index 4d7fe1a..ea27c9c
--- a/arch/arm/mach-omap2/prm2xxx_3xxx.c
+++ b/arch/arm/mach-omap2/prm2xxx_3xxx.c
@@ -189,7 +189,7 @@ int omap3_prm_vc_set_i2c_slave_addr(u8 vc_id, u8 slave_addr)
 		return -EINVAL;
 
 	omap2_prm_rmw_mod_reg_bits(vc->smps_sa_mask, 
-				   slave_addr << ffs(vc->smps_sa_mask),
+				   slave_addr << (ffs(vc->smps_sa_mask)-1),
 				   OMAP3430_GR_MOD,
 				   OMAP3_PRM_VC_SMPS_SA_OFFSET);
 
@@ -213,13 +213,13 @@ static int omap3_prm_vc_set_pmic_reg_addrs(u8 vc_id, u8 volt_addr, u8 cmd_addr)
 	struct omap3_prm_vc *vc = &vc_channels[vc_id];
 
 	omap2_prm_rmw_mod_reg_bits(vc->smps_vol_ra_mask, 
-				   volt_addr << ffs(vc->smps_vol_ra_mask),
+				   volt_addr << (ffs(vc->smps_vol_ra_mask)-1),
 				   OMAP3430_GR_MOD,
 				   OMAP3_PRM_VC_SMPS_VOL_RA_OFFSET);
 
 	if (cmd_addr)
 		omap2_prm_rmw_mod_reg_bits(vc->smps_cmd_ra_mask, 
-					cmd_addr << ffs(vc->smps_cmd_ra_mask),
+					cmd_addr << (ffs(vc->smps_cmd_ra_mask)-1),
 					OMAP3430_GR_MOD,
 					OMAP3_PRM_VC_SMPS_CMD_RA_OFFSET);
 	
diff --git a/arch/arm/mach-omap2/prm44xx.c b/arch/arm/mach-omap2/prm44xx.c
old mode 100644
new mode 100755
index 2758b75..76bf6d3
--- a/arch/arm/mach-omap2/prm44xx.c
+++ b/arch/arm/mach-omap2/prm44xx.c
@@ -234,7 +234,7 @@ int omap4_prm_vc_set_i2c_slave_addr(u8 vc_id, u8 slave_addr)
 		return -EINVAL;
 
 	omap4_prminst_rmw_inst_reg_bits(vc->smps_sa_mask, 
-					slave_addr << ffs(vc->smps_sa_mask),
+					slave_addr << (ffs(vc->smps_sa_mask)-1),
 					OMAP4430_PRM_PARTITION,
 					OMAP4430_PRM_DEVICE_INST,
 					OMAP4_PRM_VC_SMPS_SA_OFFSET);
@@ -260,14 +260,14 @@ static int omap4_prm_vc_set_pmic_reg_addrs(u8 vc_id,
 	struct omap4_prm_vc *vc = &vc_channels[vc_id];
 
 	omap4_prminst_rmw_inst_reg_bits(vc->smps_vol_ra_mask, 
-					volt_addr << ffs(vc->smps_vol_ra_mask),
+					volt_addr << (ffs(vc->smps_vol_ra_mask)-1),
 					OMAP4430_PRM_PARTITION,
 					OMAP4430_PRM_DEVICE_INST,
 					OMAP4_PRM_VC_VAL_SMPS_RA_VOL_OFFSET);
 
 	if (cmd_addr)
 		omap4_prminst_rmw_inst_reg_bits(vc->smps_cmd_ra_mask, 
-					cmd_addr << ffs(vc->smps_cmd_ra_mask),
+					cmd_addr << (ffs(vc->smps_cmd_ra_mask)-1),
 					OMAP4430_PRM_PARTITION,
 					OMAP4430_PRM_DEVICE_INST,
 					OMAP4_PRM_VC_VAL_SMPS_RA_CMD_OFFSET);

Re: Smartreflex on 'pm-wip/voltdm' Branch

[Kevin Hilman]

Vishwa, Shweta,

Vishwanath Sripathy <vishwanath.bs@ti.com> writes:

[...]

>> I am testing Smartreflex on your Branch 'pm-wip/voltdm'. There seems
>> an issue with reading VP registers.
>> For OMAP3 and OMAP4 reading debugfs entries shows constant voltage.

Thanks for testing.

[...]

> I did a quick debug on this and found that the root cause of the issue is
> in usage of ffs (because of this, i2c slave address was configured wrongly
> in vc).
> Basically ffs returns the position of the first (least significant) bit
> set in the word and the least significant bit is position 1 where as our
> bit operation assumes that least significant position is 0.

Vishwa, Thanks for findingg this.  Indeed, I had assumed ffs() was zero based,
but it is 1 based.  

> I tested the attached patch on OMAP3 and it seems to work fine.

> Kevin,
> You may want to incorporate this change in your next version if this seems
> OK to you.

Yes, I will fix this in forthcoming versions.

Thanks,

Kevin

Re: Smartreflex on 'pm-wip/voltdm' Branch

[Gulati, Shweta]

[-- Attachment #1: Type: text/plain, Size: 1276 bytes --]

Hi,

On Mon, Mar 28, 2011 at 9:57 PM, Kevin Hilman <khilman@ti.com> wrote:
> Vishwa, Shweta,
>
> Vishwanath Sripathy <vishwanath.bs@ti.com> writes:
>
> [...]
>
>>> I am testing Smartreflex on your Branch 'pm-wip/voltdm'. There seems
>>> an issue with reading VP registers.
>>> For OMAP3 and OMAP4 reading debugfs entries shows constant voltage.
>
> Thanks for testing.
>
> [...]
>
>> I did a quick debug on this and found that the root cause of the issue is
>> in usage of ffs (because of this, i2c slave address was configured wrongly
>> in vc).
>> Basically ffs returns the position of the first (least significant) bit
>> set in the word and the least significant bit is position 1 where as our
>> bit operation assumes that least significant position is 0.
>
> Vishwa, Thanks for findingg this.  Indeed, I had assumed ffs() was zero based,
> but it is 1 based.
>
>> I tested the attached patch on OMAP3 and it seems to work fine.
Attached, is Patch which fixes the Issue, It works Fine for OMAP3 and
OMAP4 SDP boards.
>> Kevin,
>> You may want to incorporate this change in your next version if this seems
>> OK to you.
>
> Yes, I will fix this in forthcoming versions.
>
> Thanks,
>
> Kevin
>



-- 
Thanks,
Regards,
Shweta

[-- Attachment #2: 0001-OMAP2-PM-Ensure-Bitshifts-are-calculated-accurately.patch --]
[-- Type: application/octet-stream, Size: 3050 bytes --]

From e97d6eb6376cbcebd057879588151e9e56b854b3 Mon Sep 17 00:00:00 2001
From: Shweta Gulati <shweta.gulati@ti.com>
Date: Wed, 30 Mar 2011 11:31:04 +0530
Subject: [PATCH] OMAP2+: PM: Ensure Bitshifts are calculated accurately

To Set I2C Slave Address and set PMIC register address used
by VC, the bit shifts are calculated by using 'ffs' function
on bit masks.With 'ffs' least significant bit is 1 instead of 0.
So addresses are wrongly set. Using '__ffs' instead of 'ffs'
where 0 is least significant bit.

It applies on top of branch 'pm-wip/voltdm'

Signed-off-by: Shweta Gulati <shweta.gulati@ti.com>
---

 arch/arm/mach-omap2/prm2xxx_3xxx.c |    6 +++---
 arch/arm/mach-omap2/prm44xx.c      |    6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-omap2/prm2xxx_3xxx.c b/arch/arm/mach-omap2/prm2xxx_3xxx.c
index 4d7fe1a..ecb15ed 100644
--- a/arch/arm/mach-omap2/prm2xxx_3xxx.c
+++ b/arch/arm/mach-omap2/prm2xxx_3xxx.c
@@ -189,7 +189,7 @@ int omap3_prm_vc_set_i2c_slave_addr(u8 vc_id, u8 slave_addr)
 		return -EINVAL;
 
 	omap2_prm_rmw_mod_reg_bits(vc->smps_sa_mask, 
-				   slave_addr << ffs(vc->smps_sa_mask),
+				   slave_addr << __ffs(vc->smps_sa_mask),
 				   OMAP3430_GR_MOD,
 				   OMAP3_PRM_VC_SMPS_SA_OFFSET);
 
@@ -213,13 +213,13 @@ static int omap3_prm_vc_set_pmic_reg_addrs(u8 vc_id, u8 volt_addr, u8 cmd_addr)
 	struct omap3_prm_vc *vc = &vc_channels[vc_id];
 
 	omap2_prm_rmw_mod_reg_bits(vc->smps_vol_ra_mask, 
-				   volt_addr << ffs(vc->smps_vol_ra_mask),
+				   volt_addr << __ffs(vc->smps_vol_ra_mask),
 				   OMAP3430_GR_MOD,
 				   OMAP3_PRM_VC_SMPS_VOL_RA_OFFSET);
 
 	if (cmd_addr)
 		omap2_prm_rmw_mod_reg_bits(vc->smps_cmd_ra_mask, 
-					cmd_addr << ffs(vc->smps_cmd_ra_mask),
+					cmd_addr << __ffs(vc->smps_cmd_ra_mask),
 					OMAP3430_GR_MOD,
 					OMAP3_PRM_VC_SMPS_CMD_RA_OFFSET);
 	
diff --git a/arch/arm/mach-omap2/prm44xx.c b/arch/arm/mach-omap2/prm44xx.c
index 2758b75..c542c25 100644
--- a/arch/arm/mach-omap2/prm44xx.c
+++ b/arch/arm/mach-omap2/prm44xx.c
@@ -234,7 +234,7 @@ int omap4_prm_vc_set_i2c_slave_addr(u8 vc_id, u8 slave_addr)
 		return -EINVAL;
 
 	omap4_prminst_rmw_inst_reg_bits(vc->smps_sa_mask, 
-					slave_addr << ffs(vc->smps_sa_mask),
+					slave_addr << __ffs(vc->smps_sa_mask),
 					OMAP4430_PRM_PARTITION,
 					OMAP4430_PRM_DEVICE_INST,
 					OMAP4_PRM_VC_SMPS_SA_OFFSET);
@@ -260,14 +260,14 @@ static int omap4_prm_vc_set_pmic_reg_addrs(u8 vc_id,
 	struct omap4_prm_vc *vc = &vc_channels[vc_id];
 
 	omap4_prminst_rmw_inst_reg_bits(vc->smps_vol_ra_mask, 
-					volt_addr << ffs(vc->smps_vol_ra_mask),
+				volt_addr << __ffs(vc->smps_vol_ra_mask),
 					OMAP4430_PRM_PARTITION,
 					OMAP4430_PRM_DEVICE_INST,
 					OMAP4_PRM_VC_VAL_SMPS_RA_VOL_OFFSET);
 
 	if (cmd_addr)
 		omap4_prminst_rmw_inst_reg_bits(vc->smps_cmd_ra_mask, 
-					cmd_addr << ffs(vc->smps_cmd_ra_mask),
+					cmd_addr << __ffs(vc->smps_cmd_ra_mask),
 					OMAP4430_PRM_PARTITION,
 					OMAP4430_PRM_DEVICE_INST,
 					OMAP4_PRM_VC_VAL_SMPS_RA_CMD_OFFSET);
-- 
1.7.0.4

Re: Smartreflex on 'pm-wip/voltdm' Branch

[Kevin Hilman]

Hi Shweta,

"Gulati, Shweta" <shweta.gulati@ti.com> writes:

[...]

>>> I did a quick debug on this and found that the root cause of the issue is
>>> in usage of ffs (because of this, i2c slave address was configured wrongly
>>> in vc).
>>> Basically ffs returns the position of the first (least significant) bit
>>> set in the word and the least significant bit is position 1 where as our
>>> bit operation assumes that least significant position is 0.
>>
>> Vishwa, Thanks for findingg this.  Indeed, I had assumed ffs() was zero based,
>> but it is 1 based.
>>
>>> I tested the attached patch on OMAP3 and it seems to work fine.
> Attached, is Patch which fixes the Issue, It works Fine for OMAP3 and
> OMAP4 SDP boards.

Thanks for the patch, but my voltage cleanup series is work-in-progress,
and a moving target at the moment.  I've changed (again) the way I'm
splitting up the code so this patch won't apply directly.

I will however change to using __ffs instead of ffs.  Thanks for
pointing that out.  I wasn't aware of the difference between __ffs() and
ffs()

Kevin


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