Re: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation
On Tue, Jun 28, 2011 at 04:59:57PM -0700, Colin Cross wrote:
On Tue, Jun 28, 2011 at 4:46 PM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
On Tue, Jun 28, 2011 at 04:37:08PM -0700, Colin Cross wrote:
I don't think it affects bogomips - loops_per_jiffy can still be
calibrated and updated by cpufreq, udelay just wont use them.
No, you can't avoid it. __delay(), udelay(), and the global
loops_per_jiffy are all linked together - for instance this is how
the spinlock code has a 1s timeout:
static void __spin_lock_debug(raw_spinlock_t *lock)
{
u64 loops = loops_per_jiffy * HZ;
int print_once = 1;
for (;;) {
for (i = 0; i loops; i++) {
if (arch_spin_trylock(lock-raw_lock))
return;
__delay(1);
}
which goes wrong for all the same reasons you're pointing out about
udelay(). So just restricting your argument to udelay() is not
realistic.
True, there are a few other users of loops_per_jiffy and __delay, but
it looks like __spin_lock_debug is the only one worth worrying about,
and it's timing is not as critical as udelay - worst case here is that
the warning occurs after 250 ms instead of 1s. Leaving
loops_per_jiffy and __delay intact, and fixing udelay, would still be
a net gain.
Other users of loops_per_jiffy are incorrect in any case:
static inline void omap_hsmmc_reset_controller_fsm(struct omap_hsmmc_host
*host, unsigned long bit)
{
unsigned long i = 0;
unsigned long limit = (loops_per_jiffy *
msecs_to_jiffies(MMC_TIMEOUT_MS));
...
if (mmc_slot(host).features HSMMC_HAS_UPDATED_RESET) {
while ((!(OMAP_HSMMC_READ(host-base, SYSCTL) bit))
(i++ limit))
cpu_relax();
}
Is rubbish.
static void omap_write_buf_pref(struct mtd_info *mtd,
const u_char *buf, int len)
{
...
/* wait for data to flushed-out before reset the prefetch */
tim = 0;
limit = (loops_per_jiffy *
msecs_to_jiffies(OMAP_NAND_TIMEOUT_MS));while
(gpmc_read_status(GPMC_PREFETCH_COUNT) (tim++ limit))
cpu_relax();
Another load of rubbish.
static int flush(struct pl022 *pl022)
{
unsigned long limit = loops_per_jiffy 1;
dev_dbg(pl022-adev-dev, flush\n);
do {
while (readw(SSP_SR(pl022-virtbase)) SSP_SR_MASK_RNE)
readw(SSP_DR(pl022-virtbase));
} while ((readw(SSP_SR(pl022-virtbase)) SSP_SR_MASK_BSY) limit--);
Yet more...
static int flush(struct driver_data *drv_data)
{
unsigned long limit = loops_per_jiffy 1;
void __iomem *reg = drv_data-ioaddr;
do {
while (read_SSSR(reg) SSSR_RNE) {
read_SSDR(reg);
}
} while ((read_SSSR(reg) SSSR_BSY) --limit);
and...
sound/soc/samsung/i2s.c:
#define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
/* Be patient */
val = msecs_to_loops(1) / 1000; /* 1 usec */
while (--val)
cpu_relax();
even worse.
#define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
static int s3c2412_snd_lrsync(struct s3c_i2sv2_info *i2s)
{
u32 iiscon;
unsigned long loops = msecs_to_loops(5);
while (--loops) {
iiscon = readl(i2s-regs + S3C2412_IISCON);
if (iiscon S3C2412_IISCON_LRINDEX)
break;
cpu_relax();
}
It just goes on...
And strangely the major offender of this stuff are ARM drivers, not other
peoples stuff and not stuff in drivers/staging.
So I don't think its sane to ignore loops_per_jiffy and __delay, and just
concentrate on udelay().
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation
On Wed, Jun 29, 2011 at 7:00 AM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
On Tue, Jun 28, 2011 at 04:59:57PM -0700, Colin Cross wrote:
On Tue, Jun 28, 2011 at 4:46 PM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
On Tue, Jun 28, 2011 at 04:37:08PM -0700, Colin Cross wrote:
I don't think it affects bogomips - loops_per_jiffy can still be
calibrated and updated by cpufreq, udelay just wont use them.
No, you can't avoid it. __delay(), udelay(), and the global
loops_per_jiffy are all linked together - for instance this is how
the spinlock code has a 1s timeout:
static void __spin_lock_debug(raw_spinlock_t *lock)
{
u64 loops = loops_per_jiffy * HZ;
int print_once = 1;
for (;;) {
for (i = 0; i loops; i++) {
if (arch_spin_trylock(lock-raw_lock))
return;
__delay(1);
}
which goes wrong for all the same reasons you're pointing out about
udelay(). So just restricting your argument to udelay() is not
realistic.
True, there are a few other users of loops_per_jiffy and __delay, but
it looks like __spin_lock_debug is the only one worth worrying about,
and it's timing is not as critical as udelay - worst case here is that
the warning occurs after 250 ms instead of 1s. Leaving
loops_per_jiffy and __delay intact, and fixing udelay, would still be
a net gain.
Other users of loops_per_jiffy are incorrect in any case:
The same conclusion I came to on a quick scan of other uses of
loops_per_jiffy...
snip
And strangely the major offender of this stuff are ARM drivers, not other
peoples stuff and not stuff in drivers/staging.
So I don't think its sane to ignore loops_per_jiffy and __delay, and just
concentrate on udelay().
But this I don't understand. Every other use I found of
loops_per_jiffy is incorrect and should be changed. Fixing udelay now
would not make them any worse - they would stay just as broken as they
were, so there is no need to couple a fix to udelay with fixing other
users of loops_per_jiffy. Why block a legitimate fix because some
other broken code uses a variable whose behavior would not change?
If you are requesting an alternate change that would allow __delay to
continue to be used to time loops while irqs are off and jiffies is
not being updated, but also allows loops_per_jiffy to change in the
middle of a loop, while not increasing the number of instructions
executed in __delay, I don't think that is possible. You could
replace __delay with a function that tests against a timer, and
loops_per_jiffy with the frequency of the counter divided by HZ, but
that would limit your spinlock spins to the frequency of the counter -
1MHz on Tegra.
Are there ever other legitimate uses of loops_per_jiffy/__delay? I
don't think even the spinlock_debug use is correct - the number of
instructions executed in the loop before the __delay call (I count 17)
is going to be much higher than the instructions in the __delay(1)
call (3 instructions). The spinlock debug timeout is already going to
be much longer than expected. It looks to me like the only legitimate
use of loops_per_jiffy is to calculate the number of loops and call
__delay(loops) (exactly what udelay does), the overhead of doing
anything else will swamp the __delay call. spinlock debug can get
away with its incorrect usage, because it doesn't really care how long
the delay is, and must have a minimum overhead.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation
On 06/28/2011 04:17 PM, Russell King - ARM Linux wrote: That's why people have proposed hardware-timer based delay loops - these screw up the bogomips value (it no longer refers to the CPU but to the timer used for the delays) and the code proposed thus far probably has a severe negative impact on ARMs running at low clock rates (the calculation cost of the number of loops to run becomes significant for CPUs below 100MHz for short delays with the existing optimized assembler, so moving it into C and introducing function pointers will only make it worse.) Am I people? ;-) The code I've proposed doesn't seem to have a negative impact on our targets even when the processor is running at 19.2 Mhz. Before and after the patches I get the same lpj value (this is all described in the commit text). I've also shown that rewriting delay.S into C doesn't negatively affect the hand optimized assembly as the before and after object code is nearly identical modulo register allocation. The only issue would be the one function pointer which I haven't heard anyone complain about until now. Even if the time to get into the __delay() routine increases by a few instructions I don't see how this harms anything as udelay() is a minimum time guarantee. We should strive to make it as close as possible to the time requested by the caller, but we shouldn't balk at the introduction of a few more cycles along the setup path. Finally, since the calibration takes into account most of the new instructions I doubt it will even be noticeable overhead to have the function pointers. What more can I do to convince you to take this patch? -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation
On Wed, Jun 29, 2011 at 11:29:29AM -0700, Stephen Boyd wrote: On 06/28/2011 04:17 PM, Russell King - ARM Linux wrote: That's why people have proposed hardware-timer based delay loops - these screw up the bogomips value (it no longer refers to the CPU but to the timer used for the delays) and the code proposed thus far probably has a severe negative impact on ARMs running at low clock rates (the calculation cost of the number of loops to run becomes significant for CPUs below 100MHz for short delays with the existing optimized assembler, so moving it into C and introducing function pointers will only make it worse.) Am I people? ;-) That depends if you're a multiple personality person! The code I've proposed doesn't seem to have a negative impact on our targets even when the processor is running at 19.2 Mhz. Before and after the patches I get the same lpj value (this is all described in the commit text). I've also shown that rewriting delay.S into C doesn't negatively affect the hand optimized assembly as the before and after object code is nearly identical modulo register allocation. The only issue would be the one function pointer which I haven't heard anyone complain about until now. Even if the time to get into the __delay() routine increases by a few instructions I don't see how this harms anything as udelay() is a minimum time guarantee. We should strive to make it as close as possible to the time requested by the caller, but we shouldn't balk at the introduction of a few more cycles along the setup path. Finally, since the calibration takes into account most of the new instructions I doubt it will even be noticeable overhead to have the function pointers. What more can I do to convince you to take this patch? What I'm aware of is that I did create a kernel-side parport jtag driver, and the limiting factor in that was udelay(), or rather udelay(1) not giving a delay of 1us but several us longer - and that was tracked down to the overhead of the CPU getting into __delay. So, having experienced that problem I'm over-sensitive towards it... -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation
resending as plain text
On Fri, Jun 24, 2011 at 6:53 AM, Sanjeev Premi pr...@ti.com wrote:
Currently, loops_per_jiffy is being calculated twice for
non-SMP processors.
- Before calling cpufreq_notify_transition()
- From within cpufreq_notify_transition()
Double adjustment leads to incorrect value being assigned to
loops_per_jiffy. This manifests as incorrect BogoMIPS in
cat /proc/cpuinfo.
The value of loops_per_jiffy needs to be calculated only
when CONFIG_SMP is true. It is the core change included
in this patch.
The patch also leverages the definition of for_each_cpu()
with and without CONFIG_SMP to consolidate the mechanism
to call cpufreq_notify_transition().
Signed-off-by: Sanjeev Premi pr...@ti.com
---
Changes since v1:
* loops_per_jiffy are updated when CONFIG_SMP is true.
* leverage definition of for_each_cpu()
Tested on OMAP3EVM with and without CONFIG_SMP.
Since the log is rather long, will be posting the log in
a follow-up mail.
arch/arm/mach-omap2/omap2plus-cpufreq.c | 27 +++
1 files changed, 15 insertions(+), 12 deletions(-)
diff --git a/arch/arm/mach-omap2/omap2plus-cpufreq.c
b/arch/arm/mach-omap2/omap2plus-cpufreq.c
index 346519e..0263cae 100644
--- a/arch/arm/mach-omap2/omap2plus-cpufreq.c
+++ b/arch/arm/mach-omap2/omap2plus-cpufreq.c
@@ -97,12 +97,8 @@ static int omap_target(struct cpufreq_policy *policy,
return ret;
/* Notify transitions */
- if (is_smp()) {
- for_each_cpu(i, policy-cpus) {
- freqs.cpu = i;
- cpufreq_notify_transition(freqs, CPUFREQ_PRECHANGE);
- }
- } else {
+ for_each_cpu(i, policy-cpus) {
+ freqs.cpu = i;
cpufreq_notify_transition(freqs, CPUFREQ_PRECHANGE);
}
@@ -114,13 +110,20 @@ static int omap_target(struct cpufreq_policy *policy,
freqs.new = omap_getspeed(policy-cpu);
+#ifdef CONFIG_SMP
+ /* Adjust jiffies before transition */
+ for_each_cpu(i, policy-cpus) {
+ unsigned long lpj = per_cpu(cpu_data, i).loops_per_jiffy;
+
+ per_cpu(cpu_data, i).loops_per_jiffy = cpufreq_scale(lpj,
+ freqs.old,
+ freqs.new);
Can't this rewrite the loops_per_jiffy for the other CPU while it is
in a udelay? If it has already calculated the number of loops
necessary, and the CPU frequency increases, it could end up returning
too early from udelay.
There were previous discussions about polling a fixed-frequency timer
for udelay on SMP systems.
+ }
+#endif
+
/* Notify transitions */
- if (is_smp()) {
- for_each_cpu(i, policy-cpus) {
- freqs.cpu = i;
- cpufreq_notify_transition(freqs, CPUFREQ_POSTCHANGE);
- }
- } else {
+ for_each_cpu(i, policy-cpus) {
+ freqs.cpu = i;
cpufreq_notify_transition(freqs, CPUFREQ_POSTCHANGE);
}
--
1.7.2.2
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation
On 6/28/2011 3:29 PM, Colin Cross wrote:
resending as plain text
On Fri, Jun 24, 2011 at 6:53 AM, Sanjeev Premipr...@ti.com wrote:
+#ifdef CONFIG_SMP
+ /* Adjust jiffies before transition */
+ for_each_cpu(i, policy-cpus) {
+ unsigned long lpj = per_cpu(cpu_data, i).loops_per_jiffy;
+
+ per_cpu(cpu_data, i).loops_per_jiffy = cpufreq_scale(lpj,
+ freqs.old,
+ freqs.new);
This isn't the right patch since it does suffer from the progressive
error. There was an updated patch on this thread from Russell which
I re-based.
Can't this rewrite the loops_per_jiffy for the other CPU while it is
in a udelay? If it has already calculated the number of loops
necessary, and the CPU frequency increases, it could end up returning
too early from udelay.
There were previous discussions about polling a fixed-frequency timer
for udelay on SMP systems.
The udelay code doesn't use the per-cpu lpj variable. It uses the global
lpj. Secondly the calibration of no. of loops to be done is
precalculateed so overwrite shouldn't impact the scenario you mentioned.
Though it has an issue where, pre-calculated loops can become short/long
based on new clock change which impacts both CPU's on OMAP, when the
other CPU is in in the middle of u-delay routine..
When CPU can scale independently, then we have bigger problem since
global lpj based udelay becomes always error prone in all scenario's.
So for the OMAP, where the whole CPU cluster is scaled together,
I don't see this as a bigger problem.
Regards
Santosh
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation
On Tue, Jun 28, 2011 at 03:29:57PM -0700, Colin Cross wrote: Can't this rewrite the loops_per_jiffy for the other CPU while it is in a udelay? If it has already calculated the number of loops necessary, and the CPU frequency increases, it could end up returning too early from udelay. udelay uses the global loops_per_jiffy. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation
resending as plain text again, sorry, mailer troubles
On Tue, Jun 28, 2011 at 3:45 PM, Santosh Shilimkar
santosh.shilim...@ti.com wrote:
On 6/28/2011 3:29 PM, Colin Cross wrote:
resending as plain text
On Fri, Jun 24, 2011 at 6:53 AM, Sanjeev Premipr...@ti.com wrote:
+#ifdef CONFIG_SMP
+ /* Adjust jiffies before transition */
+ for_each_cpu(i, policy-cpus) {
+ unsigned long lpj = per_cpu(cpu_data, i).loops_per_jiffy;
+
+ per_cpu(cpu_data, i).loops_per_jiffy = cpufreq_scale(lpj,
+ freqs.old,
+ freqs.new);
This isn't the right patch since it does suffer from the progressive
error. There was an updated patch on this thread from Russell which
I re-based.
Both patches have the same issue - they update loops_per_jiffy, which
is unsafe on an SMP system.
Can't this rewrite the loops_per_jiffy for the other CPU while it is
in a udelay? If it has already calculated the number of loops
necessary, and the CPU frequency increases, it could end up returning
too early from udelay.
There were previous discussions about polling a fixed-frequency timer
for udelay on SMP systems.
The udelay code doesn't use the per-cpu lpj variable. It uses the global
lpj. Secondly the calibration of no. of loops to be done is
precalculateed so overwrite shouldn't impact the scenario you mentioned.
Though it has an issue where, pre-calculated loops can become short/long
based on new clock change which impacts both CPU's on OMAP, when the other
CPU is in in the middle of u-delay routine..
The precalculated loops is exactly the problem I described.
udelay(100) can return in 50 microseconds if the cpu speed is doubled.
On OMAP4, frequencies can range from 350Mhz to 1.5GHz, so udelay can
be more than 4 times too short. That breaks the guarantees of udelay.
When CPU can scale independently, then we have bigger problem since
global lpj based udelay becomes always error prone in all scenario's.
So for the OMAP, where the whole CPU cluster is scaled together,
I don't see this as a bigger problem.
Regards
Santosh
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation
On Tue, Jun 28, 2011 at 3:55 PM, Russell King - ARM Linux li...@arm.linux.org.uk wrote: On Tue, Jun 28, 2011 at 03:29:57PM -0700, Colin Cross wrote: Can't this rewrite the loops_per_jiffy for the other CPU while it is in a udelay? If it has already calculated the number of loops necessary, and the CPU frequency increases, it could end up returning too early from udelay. udelay uses the global loops_per_jiffy. The problem is still the same - loops_per_jiffy applies to both CPUs, and the frequency of the other CPU cannot be changed if it is in a udelay. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation
On 6/28/2011 3:53 PM, Colin Cross wrote: On Tue, Jun 28, 2011 at 3:45 PM, Santosh Shilimkar santosh.shilim...@ti.com mailto:santosh.shilim...@ti.com wrote: [] Can't this rewrite the loops_per_jiffy for the other CPU while it is in a udelay? If it has already calculated the number of loops necessary, and the CPU frequency increases, it could end up returning too early from udelay. There were previous discussions about polling a fixed-frequency timer for udelay on SMP systems. The udelay code doesn't use the per-cpu lpj variable. It uses the global lpj. Secondly the calibration of no. of loops to be done is precalculateed so overwrite shouldn't impact the scenario you mentioned. Though it has an issue where, pre-calculated loops can become short/long based on new clock change which impacts both CPU's on OMAP, when the other CPU is in in the middle of u-delay routine.. The precalculated loops is exactly the problem I described. udelay(100) can return in 50 microseconds if the cpu speed is doubled. On OMAP4, frequencies can range from 350Mhz to 1.5GHz, so udelay can be more than 4 times too short. That breaks the guarantees of udelay. You have a point and I agree with you on above. And to fix that scenrio, the only option is to use hardware timer based u-delay() which can remain constant across the CPU freq change. Regards, Santosh -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation
On Tue, Jun 28, 2011 at 03:45:22PM -0700, Santosh Shilimkar wrote: The udelay code doesn't use the per-cpu lpj variable. It uses the global lpj. Secondly the calibration of no. of loops to be done is precalculateed so overwrite shouldn't impact the scenario you mentioned. Though it has an issue where, pre-calculated loops can become short/long based on new clock change which impacts both CPU's on OMAP, when the other CPU is in in the middle of u-delay routine.. And there's also the issue where you can start a udelay loop on one CPU, be pre-empted and end up running it on a different CPU running at a different speed. The thing to bear in mind is that udelays are approximate at best - I did some investigation into the accuracy of the loops_per_jiffy calculation, and it _will_ produce shorter delays than expected by the fact that what is being calibrated is the udelay() loop _plus_ the timer interrupt overhead. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation
On 6/28/2011 4:00 PM, Santosh Shilimkar wrote: On 6/28/2011 3:53 PM, Colin Cross wrote: On Tue, Jun 28, 2011 at 3:45 PM, Santosh Shilimkar santosh.shilim...@ti.com mailto:santosh.shilim...@ti.com wrote: [] Can't this rewrite the loops_per_jiffy for the other CPU while it is in a udelay? If it has already calculated the number of loops necessary, and the CPU frequency increases, it could end up returning too early from udelay. There were previous discussions about polling a fixed-frequency timer for udelay on SMP systems. The udelay code doesn't use the per-cpu lpj variable. It uses the global lpj. Secondly the calibration of no. of loops to be done is precalculateed so overwrite shouldn't impact the scenario you mentioned. Though it has an issue where, pre-calculated loops can become short/long based on new clock change which impacts both CPU's on OMAP, when the other CPU is in in the middle of u-delay routine.. The precalculated loops is exactly the problem I described. udelay(100) can return in 50 microseconds if the cpu speed is doubled. On OMAP4, frequencies can range from 350Mhz to 1.5GHz, so udelay can be more than 4 times too short. That breaks the guarantees of udelay. You have a point and I agree with you on above. And to fix that scenrio, the only option is to use hardware timer based u-delay() which can remain constant across the CPU freq change. Or block CPUFREQ change when any CPU which is in udelay() is done with it, which will be stupid to do :-) Regards Santosh -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation
On 6/28/2011 4:03 PM, Russell King - ARM Linux wrote: On Tue, Jun 28, 2011 at 03:45:22PM -0700, Santosh Shilimkar wrote: The udelay code doesn't use the per-cpu lpj variable. It uses the global lpj. Secondly the calibration of no. of loops to be done is precalculateed so overwrite shouldn't impact the scenario you mentioned. Though it has an issue where, pre-calculated loops can become short/long based on new clock change which impacts both CPU's on OMAP, when the other CPU is in in the middle of u-delay routine.. And there's also the issue where you can start a udelay loop on one CPU, be pre-empted and end up running it on a different CPU running at a different speed. Indeed. The thing to bear in mind is that udelays are approximate at best - I did some investigation into the accuracy of the loops_per_jiffy calculation, and it _will_ produce shorter delays than expected by the fact that what is being calibrated is the udelay() loop _plus_ the timer interrupt overhead. Sure but as Colin pointed out 4 times shorter delay will be too much. Regards Santosh -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation
On Tue, Jun 28, 2011 at 03:58:57PM -0700, Colin Cross wrote: On Tue, Jun 28, 2011 at 3:55 PM, Russell King - ARM Linux li...@arm.linux.org.uk wrote: On Tue, Jun 28, 2011 at 03:29:57PM -0700, Colin Cross wrote: Can't this rewrite the loops_per_jiffy for the other CPU while it is in a udelay? If it has already calculated the number of loops necessary, and the CPU frequency increases, it could end up returning too early from udelay. udelay uses the global loops_per_jiffy. The problem is still the same - loops_per_jiffy applies to both CPUs, and the frequency of the other CPU cannot be changed if it is in a udelay. If you have a SMP system where both CPUs scale together then you will get both CPUs being impacted, which may result in udelay() terminating well early or taking much longer than was originally intended. That's rather unavoidable with software timing loops - we could add a rw spinlock around udelay, but that would require interrupts to be disabled and that wouldn't be nice in general to have every udelay running with IRQs off. That's why people have proposed hardware-timer based delay loops - these screw up the bogomips value (it no longer refers to the CPU but to the timer used for the delays) and the code proposed thus far probably has a severe negative impact on ARMs running at low clock rates (the calculation cost of the number of loops to run becomes significant for CPUs below 100MHz for short delays with the existing optimized assembler, so moving it into C and introducing function pointers will only make it worse.) -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation
On Tue, Jun 28, 2011 at 4:17 PM, Russell King - ARM Linux li...@arm.linux.org.uk wrote: On Tue, Jun 28, 2011 at 03:58:57PM -0700, Colin Cross wrote: On Tue, Jun 28, 2011 at 3:55 PM, Russell King - ARM Linux li...@arm.linux.org.uk wrote: On Tue, Jun 28, 2011 at 03:29:57PM -0700, Colin Cross wrote: Can't this rewrite the loops_per_jiffy for the other CPU while it is in a udelay? If it has already calculated the number of loops necessary, and the CPU frequency increases, it could end up returning too early from udelay. udelay uses the global loops_per_jiffy. The problem is still the same - loops_per_jiffy applies to both CPUs, and the frequency of the other CPU cannot be changed if it is in a udelay. If you have a SMP system where both CPUs scale together then you will get both CPUs being impacted, which may result in udelay() terminating well early or taking much longer than was originally intended. That's rather unavoidable with software timing loops - we could add a rw spinlock around udelay, but that would require interrupts to be disabled and that wouldn't be nice in general to have every udelay running with IRQs off. That's why people have proposed hardware-timer based delay loops - these screw up the bogomips value (it no longer refers to the CPU but to the timer used for the delays) and the code proposed thus far probably has a severe negative impact on ARMs running at low clock rates (the calculation cost of the number of loops to run becomes significant for CPUs below 100MHz for short delays with the existing optimized assembler, so moving it into C and introducing function pointers will only make it worse.) I don't think it affects bogomips - loops_per_jiffy can still be calibrated and updated by cpufreq, udelay just wont use them. If the pointer dereference is done at the udelay() call to allow each platform to override udelay, slow platforms can continue to use the original optimized assembly with only a few extra instructions overhead on entry. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation
On Tue, Jun 28, 2011 at 04:37:08PM -0700, Colin Cross wrote:
I don't think it affects bogomips - loops_per_jiffy can still be
calibrated and updated by cpufreq, udelay just wont use them.
No, you can't avoid it. __delay(), udelay(), and the global
loops_per_jiffy are all linked together - for instance this is how
the spinlock code has a 1s timeout:
static void __spin_lock_debug(raw_spinlock_t *lock)
{
u64 loops = loops_per_jiffy * HZ;
int print_once = 1;
for (;;) {
for (i = 0; i loops; i++) {
if (arch_spin_trylock(lock-raw_lock))
return;
__delay(1);
}
which goes wrong for all the same reasons you're pointing out about
udelay(). So just restricting your argument to udelay() is not
realistic.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation
On Tue, Jun 28, 2011 at 4:46 PM, Russell King - ARM Linux
li...@arm.linux.org.uk wrote:
On Tue, Jun 28, 2011 at 04:37:08PM -0700, Colin Cross wrote:
I don't think it affects bogomips - loops_per_jiffy can still be
calibrated and updated by cpufreq, udelay just wont use them.
No, you can't avoid it. __delay(), udelay(), and the global
loops_per_jiffy are all linked together - for instance this is how
the spinlock code has a 1s timeout:
static void __spin_lock_debug(raw_spinlock_t *lock)
{
u64 loops = loops_per_jiffy * HZ;
int print_once = 1;
for (;;) {
for (i = 0; i loops; i++) {
if (arch_spin_trylock(lock-raw_lock))
return;
__delay(1);
}
which goes wrong for all the same reasons you're pointing out about
udelay(). So just restricting your argument to udelay() is not
realistic.
True, there are a few other users of loops_per_jiffy and __delay, but
it looks like __spin_lock_debug is the only one worth worrying about,
and it's timing is not as critical as udelay - worst case here is that
the warning occurs after 250 ms instead of 1s. Leaving
loops_per_jiffy and __delay intact, and fixing udelay, would still be
a net gain.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation
On Mon, Jun 27, 2011 at 10:24:43AM +0530, Premi, Sanjeev wrote: -Original Message- From: Russell King - ARM Linux [mailto:li...@arm.linux.org.uk] Sent: Sunday, June 26, 2011 12:39 AM To: Premi, Sanjeev Cc: Shilimkar, Santosh; linux-omap@vger.kernel.org; linux-arm-ker...@lists.infradead.org; Hilman, Kevin Subject: Re: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation On Sun, Jun 26, 2011 at 12:23:31AM +0530, Premi, Sanjeev wrote: [sp] I thought we were solving a problem - but this makes it look like race for addding sign-offs - which I am not interested in. No, it's called packaging the patch up and getting it ready, putting it out on the list for people to test and provide Tested-by's, acked-by's etc. [sp] Agree. Would you rather people sat on fixes doing nothing with them for a month instead, watching broken -rc after broken -rc going by? [sp] The original patch was just few hours ago... not month(s). I fail to see what the problem is you're referring to. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation
-Original Message- From: Russell King - ARM Linux [mailto:li...@arm.linux.org.uk] Sent: Sunday, June 26, 2011 12:39 AM To: Premi, Sanjeev Cc: Shilimkar, Santosh; linux-omap@vger.kernel.org; linux-arm-ker...@lists.infradead.org; Hilman, Kevin Subject: Re: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation On Sun, Jun 26, 2011 at 12:23:31AM +0530, Premi, Sanjeev wrote: [sp] I thought we were solving a problem - but this makes it look like race for addding sign-offs - which I am not interested in. No, it's called packaging the patch up and getting it ready, putting it out on the list for people to test and provide Tested-by's, acked-by's etc. [sp] Agree. Would you rather people sat on fixes doing nothing with them for a month instead, watching broken -rc after broken -rc going by? [sp] The original patch was just few hours ago... not month(s). ~sanjeev -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation
-Original Message- From: Hilman, Kevin Sent: Saturday, June 25, 2011 1:44 AM To: Russell King - ARM Linux Cc: Premi, Sanjeev; linux-omap@vger.kernel.org; linux-arm-ker...@lists.infradead.org Subject: Re: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation Russell King - ARM Linux li...@arm.linux.org.uk writes: On Fri, Jun 24, 2011 at 11:20:44PM +0530, Premi, Sanjeev wrote: I was able to test BogoMIPS calculations via /proc/cpuinfo for both with without CONFIG_SMP selected. For most part things work fine - but I do notice occassional Oops and segmentation faults while doing cat /proc/cpuinfo With CONFIG_SMP enabled, system doesn't recover from the Oops; but without SMP - I noticed segmentation faults/ BUG but system does recover. They could be unrelated - but i didn't see any of these earlier today. I will continue debug on MON. I don't think these are related to the patch - I think there's something up with your hardware. Let's take the first. [root@OMAP3EVM cpufreq]# cat /proc/cpuinfo [ 73.832366] Internal error: Oops - undefined instruction: 0 [#1] SMP Ok an undefined instruction. So... [ 73.839019] Modules linked in: [ 73.842193] CPU: 0Not tainted (3.0.0-rc3-14002-g40b6752-dirty #21) [ 73.849121] PC is at __do_fault+0x1c0/0x450 [ 73.853485] LR is at __do_fault+0x2b0/0x450 [ 73.857879] pc : [c010fa18]lr : [c010fb08] psr: 0113 [ 73.857879] sp : c7907d48 ip : fp : c5d518c0 [ 73.869873] r10: 0200 r9 : 40214000 r8 : [ 73.875335] r7 : c2692f98 r6 : c0ad7600 r5 : 87fb018f r4 : [ 73.882141] r3 : 87fb0a3e r2 : 0800 r1 : 87fb01cf r0 : c5d518c0 [ 73.888977] Flags: nzcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user [ 73.896423] Control: 10c5387d Table: 8795c019 DAC: 0015 [ 73.902435] Process cat (pid: 449, stack limit = 0xc79062f8) ... lets look at the code line: [ 74.176879] Code: e1a01005 e3a02000 ebfd1694 e59d0014 (eb07fcba) and disassemble it: 0: e1a01005mov r1, r5 4: e3a02000mov r2, #0 ; 0x0 8: ebfd1694bl 0xfff45a60 c: e59d0014ldr r0, [sp, #20] 10: eb07fcbabl 0x1ff300 There is no way that 0xeb07fcba should ever cause an undefined ARM instruction on a properly functioning system. It points at a hardware problem - are you using a socketed SoC? Is it properly socketed? Is the socket dirty? And all other questions related to hardware integrity... And in particular, since we're talking CPUfreq, are you running at a frequency that the SoC and especially the memory support? Yes. the frequencies are 300 - 800MHz range. The same board is also quite stable for 1GHz operations (tested ARM only) - with sources hosted at: http://arago-project.org/git/projects/?p=linux-omap3.git;a=summary For testing, I was changing frequencies in a tight 'forever' loop. But, as I mentioned earlier that issues could be unrelated. And the loop could be exposing something else. ~sanjeev Kevin -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation
-Original Message- From: Shilimkar, Santosh Sent: Saturday, June 25, 2011 12:18 AM To: Russell King - ARM Linux Cc: Premi, Sanjeev; linux-omap@vger.kernel.org; linux-arm-ker...@lists.infradead.org; Hilman, Kevin Subject: Re: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation Russell, On 6/24/2011 8:12 AM, Russell King - ARM Linux wrote: Right, thanks for the file. Here's the patch. [.] Notice how we adjust _both_ the per-cpu loops_per_jiffy, and that we adjust them with reference to the initial values. If you adjust lpj with reference to the last, then you _will_ build up a progressively bigger and bigger error in the value over time. Thanks Russell for the change. This change should fix the global lpj for UP machine as well when build with SMP_ON_UP. Can you have a look at below complete change which should make the BOGOMIPS happy on all OMAP2PLUS machines. Generated against Kevin's cpufreq branch. url = git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-om ap-pm.git pm-wip/cpufreq. Just compile tested with UP and SMP OMAP builds. After your review, I can give a test. Regards Santosh From 9a6154c0f68e39c4d1fbc4ef3fef5ce577ba87d4 Mon Sep 17 00:00:00 2001 From: Russell King rmk+ker...@arm.linux.org.uk Date: Fri, 24 Jun 2011 10:51:17 -0700 Subject: [PATCH] OMAP2+: CPUfreq: update lpj with refernce value to avoid progressive error. Adjust _both_ the per-cpu loops_per_jiffy and global lpj. Calibrate them with with reference to the initial values to avoid a progressively bigger and bigger error in the value over time. While at this also re-use the notifiers for UP/SMP since on UP machine or UP_ON_SMP policy-cpus mask would contain only the one CPU. Signed-off-by: Santosh Shilimkar santosh.shilim...@ti.com [santosh.shilim...@ti.com: rebased against omap cpufreq upstream branch] Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk Cc: Kevin Hilman khil...@ti.com [sp] I thought we were solving a problem - but this makes it look like race for addding sign-offs - which I am not interested in. [snip]...[snip] -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation
Right, thanks for the file. Here's the patch.
--- omap2plus-cpufreq.c~2011-06-24 15:50:32.0 +0100
+++ omap2plus-cpufreq.c 2011-06-24 16:00:08.0 +0100
@@ -44,6 +44,16 @@
static char *mpu_clk_name;
static struct device *mpu_dev;
+#ifdef CONFIG_SMP
+struct lpj_info {
+ unsigned long ref;
+ unsigned intfreq;
+};
+
+static DEFINE_PER_CPU(struct lpj_info, lpj_ref);
+static struct lpj_info global_lpj_ref;
+#endif
+
static int omap_verify_speed(struct cpufreq_policy *policy)
{
if (!freq_table)
@@ -109,14 +119,25 @@
freqs.new = omap_getspeed(policy-cpu);
#ifdef CONFIG_SMP
- /* Adjust jiffies before transition */
+ /* Adjust per-cpu loops_per_jiffy before transition */
for_each_cpu(i, policy-cpus) {
- unsigned long lpj = per_cpu(cpu_data, i).loops_per_jiffy;
-
- per_cpu(cpu_data, i).loops_per_jiffy = cpufreq_scale(lpj,
- freqs.old,
- freqs.new);
+ struct lpj_info *lpj = per_cpu(lpj_ref, i);
+ if (!lpj-freq) {
+ lpj-ref = per_cpu(cpu_data, i).loops_per_jiffy;
+ lpj-freq = freqs.old;
+ }
+
+ per_cpu(cpu_data, i).loops_per_jiffy =
+ cpufreq_scale(lpj-ref, lpj-freq, freqs.new);
+ }
+
+ /* And don't forget to adjust the global one */
+ if (!global_lpj_ref.freq) {
+ global_lpj_ref.ref = loops_per_jiffy;
+ global_lpj_ref.freq = freqs.old;
}
+ loops_per_jiffy = cpufreq_scale(global_lpj_ref.ref, global_lpj_ref.freq,
+ freqs.new);
#endif
/* Notify transitions */
Notice how we adjust _both_ the per-cpu loops_per_jiffy, and that we
adjust them with reference to the initial values.
If you adjust lpj with reference to the last, then you _will_ build up
a progressively bigger and bigger error in the value over time.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation
-Original Message-
From: Russell King - ARM Linux [mailto:li...@arm.linux.org.uk]
Sent: Friday, June 24, 2011 8:42 PM
To: Premi, Sanjeev
Cc: linux-omap@vger.kernel.org; linux-arm-ker...@lists.infradead.org
Subject: Re: [PATCHv2] omap2+: pm: cpufreq: Fix
loops_per_jiffy calculation
Right, thanks for the file. Here's the patch.
--- omap2plus-cpufreq.c~ 2011-06-24 15:50:32.0 +0100
+++ omap2plus-cpufreq.c 2011-06-24 16:00:08.0 +0100
@@ -44,6 +44,16 @@
static char *mpu_clk_name;
static struct device *mpu_dev;
+#ifdef CONFIG_SMP
+struct lpj_info {
+ unsigned long ref;
+ unsigned intfreq;
+};
+
+static DEFINE_PER_CPU(struct lpj_info, lpj_ref);
+static struct lpj_info global_lpj_ref;
+#endif
+
static int omap_verify_speed(struct cpufreq_policy *policy)
{
if (!freq_table)
@@ -109,14 +119,25 @@
freqs.new = omap_getspeed(policy-cpu);
#ifdef CONFIG_SMP
- /* Adjust jiffies before transition */
+ /* Adjust per-cpu loops_per_jiffy before transition */
for_each_cpu(i, policy-cpus) {
- unsigned long lpj = per_cpu(cpu_data,
i).loops_per_jiffy;
-
- per_cpu(cpu_data, i).loops_per_jiffy =
cpufreq_scale(lpj,
- freqs.old,
- freqs.new);
+ struct lpj_info *lpj = per_cpu(lpj_ref, i);
+ if (!lpj-freq) {
+ lpj-ref = per_cpu(cpu_data, i).loops_per_jiffy;
+ lpj-freq = freqs.old;
+ }
+
+ per_cpu(cpu_data, i).loops_per_jiffy =
+ cpufreq_scale(lpj-ref, lpj-freq, freqs.new);
+ }
+
+ /* And don't forget to adjust the global one */
+ if (!global_lpj_ref.freq) {
+ global_lpj_ref.ref = loops_per_jiffy;
+ global_lpj_ref.freq = freqs.old;
}
+ loops_per_jiffy = cpufreq_scale(global_lpj_ref.ref,
global_lpj_ref.freq,
+ freqs.new);
#endif
/* Notify transitions */
Notice how we adjust _both_ the per-cpu loops_per_jiffy, and that we
adjust them with reference to the initial values.
If you adjust lpj with reference to the last, then you _will_ build up
a progressively bigger and bigger error in the value over time.
Russell,
I was able to test BogoMIPS calculations via /proc/cpuinfo for
both with without CONFIG_SMP selected.
For most part things work fine - but I do notice occassional Oops
and segmentation faults while doing cat /proc/cpuinfo
With CONFIG_SMP enabled, system doesn't recover from the Oops;
but without SMP - I noticed segmentation faults/ BUG but system
does recover.
They could be unrelated - but i didn't see any of these earlier
today. I will continue debug on MON.
Here are details:
[1] This log comes corresponds to CONFIG_SMP enabled.
[2] This corresponds to running cat /proc/cpuinfo in a tight
loop. (CONFIG_SMP disabled).
[3] Saw only once today - but had seen it few days ago. None
of my local changes included. (CONFIG_SMP disabled).
[http://marc.info/?l=linux-omapm=130884641524123w=2]
~sanjeev
=== [1]
[root@OMAP3EVM cpufreq]# cat /proc/cpuinfo
[ 73.832366] Internal error: Oops - undefined instruction: 0 [#1] SMP
[ 73.839019] Modules linked in:
[ 73.842193] CPU: 0Not tainted (3.0.0-rc3-14002-g40b6752-dirty #21)
[ 73.849121] PC is at __do_fault+0x1c0/0x450
[ 73.853485] LR is at __do_fault+0x2b0/0x450
[ 73.857879] pc : [c010fa18]lr : [c010fb08]psr: 0113
[ 73.857879] sp : c7907d48 ip : fp : c5d518c0
[ 73.869873] r10: 0200 r9 : 40214000 r8 :
[ 73.875335] r7 : c2692f98 r6 : c0ad7600 r5 : 87fb018f r4 :
[ 73.882141] r3 : 87fb0a3e r2 : 0800 r1 : 87fb01cf r0 : c5d518c0
[ 73.888977] Flags: nzcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
[ 73.896423] Control: 10c5387d Table: 8795c019 DAC: 0015
[ 73.902435] Process cat (pid: 449, stack limit = 0xc79062f8)
[ 73.908355] Stack: (0xc7907d48 to 0xc7908000)
[ 73.912902] 7d40: 0008 c795d008
c5d4d050 c5d518f4
[ 73.921447] 7d60: 000c 40214000 c0ad7600 0002 000c
c795d008
[ 73.929992] 7d80: 40214000 c5d518c0 40214000 c2692f98 be91579c c0110760
000c
[ 73.938537] 7da0: c00bb088 0002 c5d518c0 c03111a0
c795c000
[ 73.947113] 7dc0: c795d008 0201 40214000 c5d518c0 c2692f98
be91579c c084
[ 73.955657] 7de0: c795d008 c2692f98 c7907fb0 0017 402140ea
c5d518c0 402140ea
[ 73.964202] 7e00: c5d51914 c031138c c78c6180 0002 002f
0017 c78c6180
[ 73.972747] 7e20: c08da030 c00b9184 c08da030 c00b9184 c5d518c4 c00b7860
c78c6180 c78c6180
[ 73.981292] 7e40: c04787dc c04af0e0 013e
0002
Re: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation
Russell,
On 6/24/2011 8:12 AM, Russell King - ARM Linux wrote:
Right, thanks for the file. Here's the patch.
[.]
Notice how we adjust _both_ the per-cpu loops_per_jiffy, and that we
adjust them with reference to the initial values.
If you adjust lpj with reference to the last, then you _will_ build up
a progressively bigger and bigger error in the value over time.
Thanks Russell for the change. This change should fix the global
lpj for UP machine as well when build with SMP_ON_UP.
Can you have a look at below complete change which should
make the BOGOMIPS happy on all OMAP2PLUS machines. Generated
against Kevin's cpufreq branch.
url =
git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-omap-pm.git
pm-wip/cpufreq.
Just compile tested with UP and SMP OMAP builds. After your
review, I can give a test.
Regards
Santosh
From 9a6154c0f68e39c4d1fbc4ef3fef5ce577ba87d4 Mon Sep 17 00:00:00 2001
From: Russell King rmk+ker...@arm.linux.org.uk
Date: Fri, 24 Jun 2011 10:51:17 -0700
Subject: [PATCH] OMAP2+: CPUfreq: update lpj with refernce value to
avoid progressive error.
Adjust _both_ the per-cpu loops_per_jiffy and global lpj. Calibrate them
with with reference to the initial values to avoid a progressively
bigger and bigger error in the value over time.
While at this also re-use the notifiers for UP/SMP since on
UP machine or UP_ON_SMP policy-cpus mask would contain only
the one CPU.
Signed-off-by: Santosh Shilimkar santosh.shilim...@ti.com
[santosh.shilim...@ti.com: rebased against omap cpufreq upstream branch]
Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
Cc: Kevin Hilman khil...@ti.com
---
arch/arm/mach-omap2/omap2plus-cpufreq.c | 48
+-
1 files changed, 27 insertions(+), 21 deletions(-)
diff --git a/arch/arm/mach-omap2/omap2plus-cpufreq.c
b/arch/arm/mach-omap2/omap2plus-cpufreq.c
index 1f3b2e1..434698e 100644
--- a/arch/arm/mach-omap2/omap2plus-cpufreq.c
+++ b/arch/arm/mach-omap2/omap2plus-cpufreq.c
@@ -38,6 +38,16 @@
#include mach/hardware.h
+#ifdef CONFIG_SMP
+struct lpj_info {
+ unsigned long ref;
+ unsigned intfreq;
+};
+
+static DEFINE_PER_CPU(struct lpj_info, lpj_ref);
+static struct lpj_info global_lpj_ref;
+#endif
+
static struct cpufreq_frequency_table *freq_table;
static atomic_t freq_table_users = ATOMIC_INIT(0);
static struct clk *mpu_clk;
@@ -96,11 +106,6 @@ static int omap_target(struct cpufreq_policy *policy,
if (freqs.old == freqs.new policy-cur == freqs.new)
return ret;
- if (!is_smp()) {
- cpufreq_notify_transition(freqs, CPUFREQ_PRECHANGE);
- goto set_freq;
- }
-
/* notifiers */
for_each_cpu(i, policy-cpus) {
freqs.cpu = i;
@@ -114,19 +119,7 @@ set_freq:
ret = clk_set_rate(mpu_clk, freqs.new * 1000);
- /*
-* Generic CPUFREQ driver jiffy update is under !SMP. So jiffies
-* won't get updated when UP machine cpufreq build with
-* CONFIG_SMP enabled. Below code is added only to manage that
-* scenario
-*/
freqs.new = omap_getspeed(policy-cpu);
- if (!is_smp()) {
- loops_per_jiffy =
-cpufreq_scale(loops_per_jiffy, freqs.old, freqs.new);
- cpufreq_notify_transition(freqs, CPUFREQ_POSTCHANGE);
- goto skip_lpj;
- }
#ifdef CONFIG_SMP
/*
@@ -134,10 +127,24 @@ set_freq:
* cpufreq driver. So, update the per-CPU loops_per_jiffy value
* on frequency transition. We need to update all dependent CPUs.
*/
- for_each_cpu(i, policy-cpus)
+ for_each_cpu(i, policy-cpus) {
+ struct lpj_info *lpj = per_cpu(lpj_ref, i);
+ if (!lpj-freq) {
+ lpj-ref = per_cpu(cpu_data, i).loops_per_jiffy;
+ lpj-freq = freqs.old;
+ }
+
per_cpu(cpu_data, i).loops_per_jiffy =
- cpufreq_scale(per_cpu(cpu_data, i).loops_per_jiffy,
- freqs.old, freqs.new);
+ cpufreq_scale(lpj-ref, lpj-freq, freqs.new);
+ }
+
+ /* And don't forget to adjust the global one */
+ if (!global_lpj_ref.freq) {
+ global_lpj_ref.ref = loops_per_jiffy;
+ global_lpj_ref.freq = freqs.old;
+ }
+ loops_per_jiffy = cpufreq_scale(global_lpj_ref.ref, global_lpj_ref.freq,
+ freqs.new);
#endif
/* notifiers */
@@ -146,7 +153,6 @@ set_freq:
cpufreq_notify_transition(freqs, CPUFREQ_POSTCHANGE);
}
-skip_lpj:
return ret;
}
--
1.7.4.1
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation
On Fri, Jun 24, 2011 at 11:20:44PM +0530, Premi, Sanjeev wrote: I was able to test BogoMIPS calculations via /proc/cpuinfo for both with without CONFIG_SMP selected. For most part things work fine - but I do notice occassional Oops and segmentation faults while doing cat /proc/cpuinfo With CONFIG_SMP enabled, system doesn't recover from the Oops; but without SMP - I noticed segmentation faults/ BUG but system does recover. They could be unrelated - but i didn't see any of these earlier today. I will continue debug on MON. I don't think these are related to the patch - I think there's something up with your hardware. Let's take the first. [root@OMAP3EVM cpufreq]# cat /proc/cpuinfo [ 73.832366] Internal error: Oops - undefined instruction: 0 [#1] SMP Ok an undefined instruction. So... [ 73.839019] Modules linked in: [ 73.842193] CPU: 0Not tainted (3.0.0-rc3-14002-g40b6752-dirty #21) [ 73.849121] PC is at __do_fault+0x1c0/0x450 [ 73.853485] LR is at __do_fault+0x2b0/0x450 [ 73.857879] pc : [c010fa18]lr : [c010fb08]psr: 0113 [ 73.857879] sp : c7907d48 ip : fp : c5d518c0 [ 73.869873] r10: 0200 r9 : 40214000 r8 : [ 73.875335] r7 : c2692f98 r6 : c0ad7600 r5 : 87fb018f r4 : [ 73.882141] r3 : 87fb0a3e r2 : 0800 r1 : 87fb01cf r0 : c5d518c0 [ 73.888977] Flags: nzcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user [ 73.896423] Control: 10c5387d Table: 8795c019 DAC: 0015 [ 73.902435] Process cat (pid: 449, stack limit = 0xc79062f8) ... lets look at the code line: [ 74.176879] Code: e1a01005 e3a02000 ebfd1694 e59d0014 (eb07fcba) and disassemble it: 0: e1a01005mov r1, r5 4: e3a02000mov r2, #0 ; 0x0 8: ebfd1694bl 0xfff45a60 c: e59d0014ldr r0, [sp, #20] 10: eb07fcbabl 0x1ff300 There is no way that 0xeb07fcba should ever cause an undefined ARM instruction on a properly functioning system. It points at a hardware problem - are you using a socketed SoC? Is it properly socketed? Is the socket dirty? And all other questions related to hardware integrity... -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation
Russell King - ARM Linux li...@arm.linux.org.uk writes: On Fri, Jun 24, 2011 at 11:20:44PM +0530, Premi, Sanjeev wrote: I was able to test BogoMIPS calculations via /proc/cpuinfo for both with without CONFIG_SMP selected. For most part things work fine - but I do notice occassional Oops and segmentation faults while doing cat /proc/cpuinfo With CONFIG_SMP enabled, system doesn't recover from the Oops; but without SMP - I noticed segmentation faults/ BUG but system does recover. They could be unrelated - but i didn't see any of these earlier today. I will continue debug on MON. I don't think these are related to the patch - I think there's something up with your hardware. Let's take the first. [root@OMAP3EVM cpufreq]# cat /proc/cpuinfo [ 73.832366] Internal error: Oops - undefined instruction: 0 [#1] SMP Ok an undefined instruction. So... [ 73.839019] Modules linked in: [ 73.842193] CPU: 0Not tainted (3.0.0-rc3-14002-g40b6752-dirty #21) [ 73.849121] PC is at __do_fault+0x1c0/0x450 [ 73.853485] LR is at __do_fault+0x2b0/0x450 [ 73.857879] pc : [c010fa18]lr : [c010fb08]psr: 0113 [ 73.857879] sp : c7907d48 ip : fp : c5d518c0 [ 73.869873] r10: 0200 r9 : 40214000 r8 : [ 73.875335] r7 : c2692f98 r6 : c0ad7600 r5 : 87fb018f r4 : [ 73.882141] r3 : 87fb0a3e r2 : 0800 r1 : 87fb01cf r0 : c5d518c0 [ 73.888977] Flags: nzcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user [ 73.896423] Control: 10c5387d Table: 8795c019 DAC: 0015 [ 73.902435] Process cat (pid: 449, stack limit = 0xc79062f8) ... lets look at the code line: [ 74.176879] Code: e1a01005 e3a02000 ebfd1694 e59d0014 (eb07fcba) and disassemble it: 0: e1a01005mov r1, r5 4: e3a02000mov r2, #0 ; 0x0 8: ebfd1694bl 0xfff45a60 c: e59d0014ldr r0, [sp, #20] 10: eb07fcbabl 0x1ff300 There is no way that 0xeb07fcba should ever cause an undefined ARM instruction on a properly functioning system. It points at a hardware problem - are you using a socketed SoC? Is it properly socketed? Is the socket dirty? And all other questions related to hardware integrity... And in particular, since we're talking CPUfreq, are you running at a frequency that the SoC and especially the memory support? Kevin -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation
-Original Message- From: Premi, Sanjeev Sent: Friday, June 24, 2011 7:24 PM To: linux-omap@vger.kernel.org; linux-arm-ker...@lists.infradead.org Cc: Premi, Sanjeev Subject: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation Currently, loops_per_jiffy is being calculated twice for non-SMP processors. - Before calling cpufreq_notify_transition() - From within cpufreq_notify_transition() Double adjustment leads to incorrect value being assigned to loops_per_jiffy. This manifests as incorrect BogoMIPS in cat /proc/cpuinfo. The value of loops_per_jiffy needs to be calculated only when CONFIG_SMP is true. It is the core change included in this patch. The patch also leverages the definition of for_each_cpu() with and without CONFIG_SMP to consolidate the mechanism to call cpufreq_notify_transition(). Signed-off-by: Sanjeev Premi pr...@ti.com --- Changes since v1: * loops_per_jiffy are updated when CONFIG_SMP is true. * leverage definition of for_each_cpu() Tested on OMAP3EVM with and without CONFIG_SMP. Since the log is rather long, will be posting the log in a follow-up mail. [snip]...[snip] Snapshot of test log - with and without SMP included here: To ensure compile-time and run-time checks for SMP are visible, the patch was tested with this code inserted at enty of function omap_target(). #ifdef CONFIG_SMP printk (KERN_ERR I am defined SMP!\n); #else printk (KERN_ERR I am not defined SMP!\n); #endif if (is_smp()) printk (KERN_ERR Runtime evaluating to SMP!\n); else printk (KERN_ERR Runtime evaluating to no SMP!\n); With CONFIG_SMP disabled [root@OMAP3EVM /]# cd /sys/devices/system/cpu/cpu0/cpufreq/ [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# cat scaling_available_frequencies 30 60 80 100 [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# echo 30 scaling_setspeed [ 56.278137] I am not defined SMP! [ 56.281768] Runtime evaluating to no SMP! [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# cat cpuinfo_cur_freq 30 [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# cat /proc/cpuinfo Processor : ARMv7 Processor rev 2 (v7l) BogoMIPS: 298.32 Features: swp half thumb fastmult vfp edsp thumbee neon vfpv3 CPU implementer : 0x41 CPU architecture: 7 CPU variant : 0x3 CPU part: 0xc08 CPU revision: 2 Hardware: OMAP3 EVM Revision: 0020 Serial : [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# echo 80 scaling_setspeed [ 96.938049] I am not defined SMP! [ 96.941833] Runtime evaluating to no SMP! [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# cat /proc/cpuinfo Processor : ARMv7 Processor rev 2 (v7l) BogoMIPS: 796.19 Features: swp half thumb fastmult vfp edsp thumbee neon vfpv3 CPU implementer : 0x41 CPU architecture: 7 CPU variant : 0x3 CPU part: 0xc08 CPU revision: 2 Hardware: OMAP3 EVM Revision: 0020 Serial : [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# echo 10 scaling_setspeed [ 110.865631] I am not defined SMP! [ 110.870025] Runtime evaluating to no SMP! [root@OMAP3EVM cpufreq]# echo 100 scaling_setspeed [ 116.258941] I am not defined SMP! [ 116.262725] Runtime evaluating to no SMP! [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# cat cpuinfo_cur_freq 100 [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# cat /proc/cpuinfo Processor : ARMv7 Processor rev 2 (v7l) BogoMIPS: 996.74 Features: swp half thumb fastmult vfp edsp thumbee neon vfpv3 CPU implementer : 0x41 CPU architecture: 7 CPU variant : 0x3 CPU part: 0xc08 CPU revision: 2 Hardware: OMAP3 EVM Revision: 0020 Serial : [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# With CONFIG_SMP enabled [root@OMAP3EVM /]# cd /sys/devices/system/cpu/cpu0/cpufreq/ [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# cat scaling_available_frequencies 30 60 80 100 [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# echo 30 scaling_setspeed [ 25.040496] I am defined SMP! [ 25.043884] Runtime evaluating to no SMP! [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# [root@OMAP3EVM cpufreq]# cat /proc/cpuinfo Processor : ARMv7 Processor rev 2 (v7l) processor : 0 BogoMIPS: 298.32 Features: swp half
Re: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation
On 6/24/2011 7:23 PM, Sanjeev Premi wrote: Currently, loops_per_jiffy is being calculated twice for non-SMP processors. - Before calling cpufreq_notify_transition() - From within cpufreq_notify_transition() Double adjustment leads to incorrect value being assigned to loops_per_jiffy. This manifests as incorrect BogoMIPS in cat /proc/cpuinfo. The value of loops_per_jiffy needs to be calculated only when CONFIG_SMP is true. It is the core change included in this patch. The patch also leverages the definition of for_each_cpu() with and without CONFIG_SMP to consolidate the mechanism to call cpufreq_notify_transition(). Signed-off-by: Sanjeev Premipr...@ti.com NAK. This patch again doesn't make sense considering your issue. Also jiffies should not be undated before changing the freq. If the set_rate failed for some reason then you will have wrong jiffies value. I understand your issue now. The code for global lpj updation should have been checking smp_on_up() instead of is_smp(). That one line change is enough. I will post a patch on the same once I reach to office. Regards Santosh -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation
-Original Message- From: Shilimkar, Santosh Sent: Friday, June 24, 2011 8:05 PM To: Premi, Sanjeev Cc: linux-omap@vger.kernel.org; linux-arm-ker...@lists.infradead.org Subject: Re: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation On 6/24/2011 7:23 PM, Sanjeev Premi wrote: Currently, loops_per_jiffy is being calculated twice for non-SMP processors. - Before calling cpufreq_notify_transition() - From within cpufreq_notify_transition() Double adjustment leads to incorrect value being assigned to loops_per_jiffy. This manifests as incorrect BogoMIPS in cat /proc/cpuinfo. The value of loops_per_jiffy needs to be calculated only when CONFIG_SMP is true. It is the core change included in this patch. The patch also leverages the definition of for_each_cpu() with and without CONFIG_SMP to consolidate the mechanism to call cpufreq_notify_transition(). Signed-off-by: Sanjeev Premipr...@ti.com NAK. This patch again doesn't make sense considering your issue. Also jiffies should not be undated before changing the freq. If the set_rate failed for some reason then you will have wrong jiffies value. I understand your issue now. The code for global lpj updation should have been checking smp_on_up() instead of is_smp(). That one line change is enough. I will post a patch on the same once I reach to office. [sp] Suggest reading the patch before NAKing. The patch is updating the loops_per_jiffy value AFTER the frequency change has happenned and BEFORE post change notification is sent. ~sanjeev Regards Santosh -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation
On 6/24/2011 7:40 AM, Premi, Sanjeev wrote: -Original Message- From: Shilimkar, Santosh Sent: Friday, June 24, 2011 8:05 PM To: Premi, Sanjeev Cc: linux-omap@vger.kernel.org; linux-arm-ker...@lists.infradead.org Subject: Re: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation On 6/24/2011 7:23 PM, Sanjeev Premi wrote: Currently, loops_per_jiffy is being calculated twice for non-SMP processors. - Before calling cpufreq_notify_transition() - From within cpufreq_notify_transition() Double adjustment leads to incorrect value being assigned to loops_per_jiffy. This manifests as incorrect BogoMIPS in cat /proc/cpuinfo. The value of loops_per_jiffy needs to be calculated only when CONFIG_SMP is true. It is the core change included in this patch. The patch also leverages the definition of for_each_cpu() with and without CONFIG_SMP to consolidate the mechanism to call cpufreq_notify_transition(). Signed-off-by: Sanjeev Premipr...@ti.com NAK. This patch again doesn't make sense considering your issue. Also jiffies should not be undated before changing the freq. If the set_rate failed for some reason then you will have wrong jiffies value. I understand your issue now. The code for global lpj updation should have been checking smp_on_up() instead of is_smp(). That one line change is enough. I will post a patch on the same once I reach to office. [sp] Suggest reading the patch before NAKing. The patch is updating the loops_per_jiffy value AFTER the frequency change has happenned and BEFORE post change notification is sent. Sure. Let me respond to you with the patch instead of making more noise on the list. Regard Santosh -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCHv2] omap2+: pm: cpufreq: Fix loops_per_jiffy calculation
-Original Message-
From: Russell King - ARM Linux [mailto:li...@arm.linux.org.uk]
Sent: Friday, June 24, 2011 8:42 PM
To: Premi, Sanjeev
Cc: linux-omap@vger.kernel.org; linux-arm-ker...@lists.infradead.org
Subject: Re: [PATCHv2] omap2+: pm: cpufreq: Fix
loops_per_jiffy calculation
Right, thanks for the file. Here's the patch.
--- omap2plus-cpufreq.c~ 2011-06-24 15:50:32.0 +0100
+++ omap2plus-cpufreq.c 2011-06-24 16:00:08.0 +0100
@@ -44,6 +44,16 @@
static char *mpu_clk_name;
static struct device *mpu_dev;
+#ifdef CONFIG_SMP
+struct lpj_info {
+ unsigned long ref;
+ unsigned intfreq;
+};
+
+static DEFINE_PER_CPU(struct lpj_info, lpj_ref);
+static struct lpj_info global_lpj_ref;
+#endif
+
static int omap_verify_speed(struct cpufreq_policy *policy)
{
if (!freq_table)
@@ -109,14 +119,25 @@
freqs.new = omap_getspeed(policy-cpu);
#ifdef CONFIG_SMP
- /* Adjust jiffies before transition */
+ /* Adjust per-cpu loops_per_jiffy before transition */
for_each_cpu(i, policy-cpus) {
- unsigned long lpj = per_cpu(cpu_data,
i).loops_per_jiffy;
-
- per_cpu(cpu_data, i).loops_per_jiffy =
cpufreq_scale(lpj,
- freqs.old,
- freqs.new);
+ struct lpj_info *lpj = per_cpu(lpj_ref, i);
+ if (!lpj-freq) {
+ lpj-ref = per_cpu(cpu_data, i).loops_per_jiffy;
+ lpj-freq = freqs.old;
+ }
+
+ per_cpu(cpu_data, i).loops_per_jiffy =
+ cpufreq_scale(lpj-ref, lpj-freq, freqs.new);
+ }
+
+ /* And don't forget to adjust the global one */
+ if (!global_lpj_ref.freq) {
+ global_lpj_ref.ref = loops_per_jiffy;
+ global_lpj_ref.freq = freqs.old;
}
+ loops_per_jiffy = cpufreq_scale(global_lpj_ref.ref,
global_lpj_ref.freq,
+ freqs.new);
#endif
/* Notify transitions */
Notice how we adjust _both_ the per-cpu loops_per_jiffy, and that we
adjust them with reference to the initial values.
If you adjust lpj with reference to the last, then you _will_ build up
a progressively bigger and bigger error in the value over time.
Russell,
I definitely didn't see so many things through your comments. But
that may just be reflection of my naivety with SMP!
I am currently testing another patch for beagle - will apply and
test on OMAP3EVM (just to be sure).
Can I include it in my next patch rev?
~sanjeev
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
