Signed-off-by: Felipe Contreras <[email protected]>
---
arch/arm/lib/delay.S | 16 ----------------
1 files changed, 0 insertions(+), 16 deletions(-)
diff --git a/arch/arm/lib/delay.S b/arch/arm/lib/delay.S
index 8d6a876..b741eb4 100644
--- a/arch/arm/lib/delay.S
+++ b/arch/arm/lib/delay.S
@@ -42,22 +42,6 @@ ENTRY(__const_udelay) @ 0 <= r0 <= 0x7fffff06
@ Delay routine
ENTRY(__delay)
subs r0, r0, #1
-#if 0
- movls pc, lr
- subs r0, r0, #1
- movls pc, lr
- subs r0, r0, #1
- movls pc, lr
- subs r0, r0, #1
- movls pc, lr
- subs r0, r0, #1
- movls pc, lr
- subs r0, r0, #1
- movls pc, lr
- subs r0, r0, #1
- movls pc, lr
- subs r0, r0, #1
-#endif
bhi __delay
mov pc, lr
ENDPROC(__udelay)
--
1.6.5.rc1
Dne Ne 13. září 2009 23:05:59 Felipe Contreras napsal(a):
> Signed-off-by: Felipe Contreras <[email protected]>
> ---
> arch/arm/lib/delay.S | 16 ----------------
> 1 files changed, 0 insertions(+), 16 deletions(-)
>
> diff --git a/arch/arm/lib/delay.S b/arch/arm/lib/delay.S
> index 8d6a876..b741eb4 100644
> --- a/arch/arm/lib/delay.S
> +++ b/arch/arm/lib/delay.S
> @@ -42,22 +42,6 @@ ENTRY(__const_udelay) @ 0 <= r0 <= 0x7fffff06
> @ Delay routine
> ENTRY(__delay)
> subs r0, r0, #1
> -#if 0
> - movls pc, lr
> - subs r0, r0, #1
> - movls pc, lr
> - subs r0, r0, #1
> - movls pc, lr
> - subs r0, r0, #1
> - movls pc, lr
> - subs r0, r0, #1
> - movls pc, lr
> - subs r0, r0, #1
> - movls pc, lr
> - subs r0, r0, #1
> - movls pc, lr
> - subs r0, r0, #1
> -#endif
> bhi __delay
> mov pc, lr
> ENDPROC(__udelay)
>
Hi
why was this code there in the first place ?
On Sun, Sep 13, 2009 at 11:28:47PM +0200, Marek Vasut wrote:
> > bhi __delay
> > mov pc, lr
> > ENDPROC(__udelay)
> >
> Hi
>
> why was this code there in the first place ?
To make the delay loop more stable and predictable on older CPUs.
Russell King - ARM Linux wrote:
> On Sun, Sep 13, 2009 at 11:28:47PM +0200, Marek Vasut wrote:
> > > bhi __delay
> > > mov pc, lr
> > > ENDPROC(__udelay)
> > >
> > Hi
> >
> > why was this code there in the first place ?
>
> To make the delay loop more stable and predictable on older CPUs.
So why has it been commented out, if it's needed for that?
-- Jamie
On Mon, Sep 14, 2009 at 01:21:00AM +0100, Jamie Lokier wrote:
> Russell King - ARM Linux wrote:
> > On Sun, Sep 13, 2009 at 11:28:47PM +0200, Marek Vasut wrote:
> > > > bhi __delay
> > > > mov pc, lr
> > > > ENDPROC(__udelay)
> > > >
> > > Hi
> > >
> > > why was this code there in the first place ?
> >
> > To make the delay loop more stable and predictable on older CPUs.
>
> So why has it been commented out, if it's needed for that?
We moved on and it penalises later CPUs, leading to udelay providing
shorter delays than requested.
So the choice was either stable and predictable on older CPUs but
buggy on newer CPUs, or correct on all CPUs but gives unnecessarily
longer delays on older CPUs.
On Mon, Sep 14, 2009 at 11:10 AM, Russell King - ARM Linux
<[email protected]> wrote:
> On Mon, Sep 14, 2009 at 01:21:00AM +0100, Jamie Lokier wrote:
>> Russell King - ARM Linux wrote:
>> > On Sun, Sep 13, 2009 at 11:28:47PM +0200, Marek Vasut wrote:
>> > > > bhi __delay
>> > > > mov pc, lr
>> > > > ENDPROC(__udelay)
>> > > >
>> > > Hi
>> > >
>> > > why was this code there in the first place ?
>> >
>> > To make the delay loop more stable and predictable on older CPUs.
>>
>> So why has it been commented out, if it's needed for that?
>
> We moved on and it penalises later CPUs, leading to udelay providing
> shorter delays than requested.
>
> So the choice was either stable and predictable on older CPUs but
> buggy on newer CPUs, or correct on all CPUs but gives unnecessarily
> longer delays on older CPUs.
Why not add an #ifdef CPU_V4 or whatever?
--
Felipe Contreras
On Mon, Sep 14, 2009 at 03:58:24PM +0300, Felipe Contreras wrote:
> On Mon, Sep 14, 2009 at 11:10 AM, Russell King - ARM Linux
> <[email protected]> wrote:
> > On Mon, Sep 14, 2009 at 01:21:00AM +0100, Jamie Lokier wrote:
> >> Russell King - ARM Linux wrote:
> >> > On Sun, Sep 13, 2009 at 11:28:47PM +0200, Marek Vasut wrote:
> >> > > > ? ? ? ? ? ? ? ? bhi ? ? __delay
> >> > > > ? ? ? ? ? ? ? ? mov ? ? pc, lr
> >> > > > ?ENDPROC(__udelay)
> >> > > >
> >> > > Hi
> >> > >
> >> > > why was this code there in the first place ?
> >> >
> >> > To make the delay loop more stable and predictable on older CPUs.
> >>
> >> So why has it been commented out, if it's needed for that?
> >
> > We moved on and it penalises later CPUs, leading to udelay providing
> > shorter delays than requested.
> >
> > So the choice was either stable and predictable on older CPUs but
> > buggy on newer CPUs, or correct on all CPUs but gives unnecessarily
> > longer delays on older CPUs.
>
> Why not add an #ifdef CPU_V4 or whatever?
Because then you get it whenever you configure for V4 as the lowest
denominator CPU, which leads to the buggy behaviour on better CPUs.
It's far better to leave it as is and just accept that the old CPUs
will have longer than necessary delays. If people really really
care (and there's likely to only be a small minority of them now)
changing the '0' to a '1' is a very simple change for them to carry
in their local tree. Unlike getting the right unrolling etc.
On Mon, Sep 14, 2009 at 5:00 PM, Russell King - ARM Linux
<[email protected]> wrote:
> On Mon, Sep 14, 2009 at 03:58:24PM +0300, Felipe Contreras wrote:
>> On Mon, Sep 14, 2009 at 11:10 AM, Russell King - ARM Linux
>> <[email protected]> wrote:
>> > On Mon, Sep 14, 2009 at 01:21:00AM +0100, Jamie Lokier wrote:
>> >> Russell King - ARM Linux wrote:
>> >> > On Sun, Sep 13, 2009 at 11:28:47PM +0200, Marek Vasut wrote:
>> >> > > > bhi __delay
>> >> > > > mov pc, lr
>> >> > > > ENDPROC(__udelay)
>> >> > > >
>> >> > > Hi
>> >> > >
>> >> > > why was this code there in the first place ?
>> >> >
>> >> > To make the delay loop more stable and predictable on older CPUs.
>> >>
>> >> So why has it been commented out, if it's needed for that?
>> >
>> > We moved on and it penalises later CPUs, leading to udelay providing
>> > shorter delays than requested.
>> >
>> > So the choice was either stable and predictable on older CPUs but
>> > buggy on newer CPUs, or correct on all CPUs but gives unnecessarily
>> > longer delays on older CPUs.
>>
>> Why not add an #ifdef CPU_V4 or whatever?
>
> Because then you get it whenever you configure for V4 as the lowest
> denominator CPU, which leads to the buggy behaviour on better CPUs.
> It's far better to leave it as is and just accept that the old CPUs
> will have longer than necessary delays. If people really really
> care (and there's likely to only be a small minority of them now)
> changing the '0' to a '1' is a very simple change for them to carry
> in their local tree. Unlike getting the right unrolling etc.
Well, they can also 'git revert' this patch. If somebody really cares
I think they should shout now and provide a better patch, otherwise
this one should be merged.
--
Felipe Contreras
On Mon, Sep 14, 2009 at 05:38:32PM +0300, Felipe Contreras wrote:
> On Mon, Sep 14, 2009 at 5:00 PM, Russell King - ARM Linux
> <[email protected]> wrote:
> > On Mon, Sep 14, 2009 at 03:58:24PM +0300, Felipe Contreras wrote:
> >> On Mon, Sep 14, 2009 at 11:10 AM, Russell King - ARM Linux
> >> <[email protected]> wrote:
> >> > On Mon, Sep 14, 2009 at 01:21:00AM +0100, Jamie Lokier wrote:
> >> >> Russell King - ARM Linux wrote:
> >> >> > On Sun, Sep 13, 2009 at 11:28:47PM +0200, Marek Vasut wrote:
> >> >> > > > ? ? ? ? ? ? ? ? bhi ? ? __delay
> >> >> > > > ? ? ? ? ? ? ? ? mov ? ? pc, lr
> >> >> > > > ?ENDPROC(__udelay)
> >> >> > > >
> >> >> > > Hi
> >> >> > >
> >> >> > > why was this code there in the first place ?
> >> >> >
> >> >> > To make the delay loop more stable and predictable on older CPUs.
> >> >>
> >> >> So why has it been commented out, if it's needed for that?
> >> >
> >> > We moved on and it penalises later CPUs, leading to udelay providing
> >> > shorter delays than requested.
> >> >
> >> > So the choice was either stable and predictable on older CPUs but
> >> > buggy on newer CPUs, or correct on all CPUs but gives unnecessarily
> >> > longer delays on older CPUs.
> >>
> >> Why not add an #ifdef CPU_V4 or whatever?
> >
> > Because then you get it whenever you configure for V4 as the lowest
> > denominator CPU, which leads to the buggy behaviour on better CPUs.
> > It's far better to leave it as is and just accept that the old CPUs
> > will have longer than necessary delays. ?If people really really
> > care (and there's likely to only be a small minority of them now)
> > changing the '0' to a '1' is a very simple change for them to carry
> > in their local tree. ?Unlike getting the right unrolling etc.
>
> Well, they can also 'git revert' this patch. If somebody really cares
> I think they should shout now and provide a better patch, otherwise
> this one should be merged.
On the other hand, having the code there as it currently stands is not
harmful in any way, so leaving it there is just as easy.
On Mon, Sep 14, 2009 at 5:40 PM, Russell King - ARM Linux
<[email protected]> wrote:
> On Mon, Sep 14, 2009 at 05:38:32PM +0300, Felipe Contreras wrote:
>> On Mon, Sep 14, 2009 at 5:00 PM, Russell King - ARM Linux
>> <[email protected]> wrote:
>> > On Mon, Sep 14, 2009 at 03:58:24PM +0300, Felipe Contreras wrote:
>> >> On Mon, Sep 14, 2009 at 11:10 AM, Russell King - ARM Linux
>> >> <[email protected]> wrote:
>> >> > On Mon, Sep 14, 2009 at 01:21:00AM +0100, Jamie Lokier wrote:
>> >> >> Russell King - ARM Linux wrote:
>> >> >> > On Sun, Sep 13, 2009 at 11:28:47PM +0200, Marek Vasut wrote:
>> >> >> > > > bhi __delay
>> >> >> > > > mov pc, lr
>> >> >> > > > ENDPROC(__udelay)
>> >> >> > > >
>> >> >> > > Hi
>> >> >> > >
>> >> >> > > why was this code there in the first place ?
>> >> >> >
>> >> >> > To make the delay loop more stable and predictable on older CPUs.
>> >> >>
>> >> >> So why has it been commented out, if it's needed for that?
>> >> >
>> >> > We moved on and it penalises later CPUs, leading to udelay providing
>> >> > shorter delays than requested.
>> >> >
>> >> > So the choice was either stable and predictable on older CPUs but
>> >> > buggy on newer CPUs, or correct on all CPUs but gives unnecessarily
>> >> > longer delays on older CPUs.
>> >>
>> >> Why not add an #ifdef CPU_V4 or whatever?
>> >
>> > Because then you get it whenever you configure for V4 as the lowest
>> > denominator CPU, which leads to the buggy behaviour on better CPUs.
>> > It's far better to leave it as is and just accept that the old CPUs
>> > will have longer than necessary delays. If people really really
>> > care (and there's likely to only be a small minority of them now)
>> > changing the '0' to a '1' is a very simple change for them to carry
>> > in their local tree. Unlike getting the right unrolling etc.
>>
>> Well, they can also 'git revert' this patch. If somebody really cares
>> I think they should shout now and provide a better patch, otherwise
>> this one should be merged.
>
> On the other hand, having the code there as it currently stands is not
> harmful in any way, so leaving it there is just as easy.
It makes the code less understandable. I'm not sure about linux's
practices, but an #if 0 generally means somebody is being lazy.
--
Felipe Contreras
On Mon, Sep 14, 2009 at 06:14:08PM +0300, Felipe Contreras wrote:
> On Mon, Sep 14, 2009 at 5:40 PM, Russell King - ARM Linux
> <[email protected]> wrote:
> > On Mon, Sep 14, 2009 at 05:38:32PM +0300, Felipe Contreras wrote:
> >> On Mon, Sep 14, 2009 at 5:00 PM, Russell King - ARM Linux
> >> <[email protected]> wrote:
> >> > On Mon, Sep 14, 2009 at 03:58:24PM +0300, Felipe Contreras wrote:
> >> >> On Mon, Sep 14, 2009 at 11:10 AM, Russell King - ARM Linux
> >> >> <[email protected]> wrote:
> >> >> > On Mon, Sep 14, 2009 at 01:21:00AM +0100, Jamie Lokier wrote:
> >> >> >> Russell King - ARM Linux wrote:
> >> >> >> > On Sun, Sep 13, 2009 at 11:28:47PM +0200, Marek Vasut wrote:
> >> >> >> > > > ? ? ? ? ? ? ? ? bhi ? ? __delay
> >> >> >> > > > ? ? ? ? ? ? ? ? mov ? ? pc, lr
> >> >> >> > > > ?ENDPROC(__udelay)
> >> >> >> > > >
> >> >> >> > > Hi
> >> >> >> > >
> >> >> >> > > why was this code there in the first place ?
> >> >> >> >
> >> >> >> > To make the delay loop more stable and predictable on older CPUs.
> >> >> >>
> >> >> >> So why has it been commented out, if it's needed for that?
> >> >> >
> >> >> > We moved on and it penalises later CPUs, leading to udelay providing
> >> >> > shorter delays than requested.
> >> >> >
> >> >> > So the choice was either stable and predictable on older CPUs but
> >> >> > buggy on newer CPUs, or correct on all CPUs but gives unnecessarily
> >> >> > longer delays on older CPUs.
> >> >>
> >> >> Why not add an #ifdef CPU_V4 or whatever?
> >> >
> >> > Because then you get it whenever you configure for V4 as the lowest
> >> > denominator CPU, which leads to the buggy behaviour on better CPUs.
> >> > It's far better to leave it as is and just accept that the old CPUs
> >> > will have longer than necessary delays. ?If people really really
> >> > care (and there's likely to only be a small minority of them now)
> >> > changing the '0' to a '1' is a very simple change for them to carry
> >> > in their local tree. ?Unlike getting the right unrolling etc.
> >>
> >> Well, they can also 'git revert' this patch. If somebody really cares
> >> I think they should shout now and provide a better patch, otherwise
> >> this one should be merged.
> >
> > On the other hand, having the code there as it currently stands is not
> > harmful in any way, so leaving it there is just as easy.
>
> It makes the code less understandable. I'm not sure about linux's
> practices, but an #if 0 generally means somebody is being lazy.
I would agree with you if it was a complicated bit of code, but it
isn't. It is a simple count to zero (or overflow) and terminate
loop. And it's certainly not about me being lazy.
Unless there is a strong argument for removing it, the code stays as
is.
So far, the argument is basically "it's a #if 0, we must get rid of
it" which is a religous argument, not a technical one. The fact is
that (as I said above) keeping it there provides the code for when
people want to enable it. That's a technical reason for keeping it.
Please can we now move to something more productive instead of this
religous argument?
On Mon, Sep 14, 2009 at 06:14:08PM +0300, Felipe Contreras wrote:
> On Mon, Sep 14, 2009 at 5:40 PM, Russell King - ARM Linux
> <[email protected]> wrote:
> > On Mon, Sep 14, 2009 at 05:38:32PM +0300, Felipe Contreras wrote:
> >> On Mon, Sep 14, 2009 at 5:00 PM, Russell King - ARM Linux
> >> <[email protected]> wrote:
> >> > On Mon, Sep 14, 2009 at 03:58:24PM +0300, Felipe Contreras wrote:
> >> >> On Mon, Sep 14, 2009 at 11:10 AM, Russell King - ARM Linux
> >> >> <[email protected]> wrote:
> >> >> > On Mon, Sep 14, 2009 at 01:21:00AM +0100, Jamie Lokier wrote:
> >> >> >> Russell King - ARM Linux wrote:
> >> >> >> > On Sun, Sep 13, 2009 at 11:28:47PM +0200, Marek Vasut wrote:
> >> >> >> > > > ? ? ? ? ? ? ? ? bhi ? ? __delay
> >> >> >> > > > ? ? ? ? ? ? ? ? mov ? ? pc, lr
> >> >> >> > > > ?ENDPROC(__udelay)
> >> >> >> > > >
> >> >> >> > > Hi
> >> >> >> > >
> >> >> >> > > why was this code there in the first place ?
> >> >> >> >
> >> >> >> > To make the delay loop more stable and predictable on older CPUs.
> >> >> >>
> >> >> >> So why has it been commented out, if it's needed for that?
> >> >> >
> >> >> > We moved on and it penalises later CPUs, leading to udelay providing
> >> >> > shorter delays than requested.
> >> >> >
> >> >> > So the choice was either stable and predictable on older CPUs but
> >> >> > buggy on newer CPUs, or correct on all CPUs but gives unnecessarily
> >> >> > longer delays on older CPUs.
> >> >>
> >> >> Why not add an #ifdef CPU_V4 or whatever?
> >> >
> >> > Because then you get it whenever you configure for V4 as the lowest
> >> > denominator CPU, which leads to the buggy behaviour on better CPUs.
> >> > It's far better to leave it as is and just accept that the old CPUs
> >> > will have longer than necessary delays. ?If people really really
> >> > care (and there's likely to only be a small minority of them now)
> >> > changing the '0' to a '1' is a very simple change for them to carry
> >> > in their local tree. ?Unlike getting the right unrolling etc.
> >>
> >> Well, they can also 'git revert' this patch. If somebody really cares
> >> I think they should shout now and provide a better patch, otherwise
> >> this one should be merged.
> >
> > On the other hand, having the code there as it currently stands is not
> > harmful in any way, so leaving it there is just as easy.
>
> It makes the code less understandable. I'm not sure about linux's
> practices, but an #if 0 generally means somebody is being lazy.
And what about a comment like "Try including the following code if ..."? I
think I saw this somewhere else in the kernel and I'd be fine with that. So,
while I generally agree that "#if 0" looks suspicious, there are a few cases
for it, though they need documentation IMHO.
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Mon, Sep 14, 2009 at 6:21 PM, Russell King - ARM Linux
<[email protected]> wrote:
> On Mon, Sep 14, 2009 at 06:14:08PM +0300, Felipe Contreras wrote:
>> On Mon, Sep 14, 2009 at 5:40 PM, Russell King - ARM Linux
>> <[email protected]> wrote:
>> > On Mon, Sep 14, 2009 at 05:38:32PM +0300, Felipe Contreras wrote:
>> >> On Mon, Sep 14, 2009 at 5:00 PM, Russell King - ARM Linux
>> >> <[email protected]> wrote:
>> >> > On Mon, Sep 14, 2009 at 03:58:24PM +0300, Felipe Contreras wrote:
>> >> >> On Mon, Sep 14, 2009 at 11:10 AM, Russell King - ARM Linux
>> >> >> <[email protected]> wrote:
>> >> >> > On Mon, Sep 14, 2009 at 01:21:00AM +0100, Jamie Lokier wrote:
>> >> >> >> Russell King - ARM Linux wrote:
>> >> >> >> > On Sun, Sep 13, 2009 at 11:28:47PM +0200, Marek Vasut wrote:
>> >> >> >> > > > bhi __delay
>> >> >> >> > > > mov pc, lr
>> >> >> >> > > > ENDPROC(__udelay)
>> >> >> >> > > >
>> >> >> >> > > Hi
>> >> >> >> > >
>> >> >> >> > > why was this code there in the first place ?
>> >> >> >> >
>> >> >> >> > To make the delay loop more stable and predictable on older CPUs.
>> >> >> >>
>> >> >> >> So why has it been commented out, if it's needed for that?
>> >> >> >
>> >> >> > We moved on and it penalises later CPUs, leading to udelay providing
>> >> >> > shorter delays than requested.
>> >> >> >
>> >> >> > So the choice was either stable and predictable on older CPUs but
>> >> >> > buggy on newer CPUs, or correct on all CPUs but gives unnecessarily
>> >> >> > longer delays on older CPUs.
>> >> >>
>> >> >> Why not add an #ifdef CPU_V4 or whatever?
>> >> >
>> >> > Because then you get it whenever you configure for V4 as the lowest
>> >> > denominator CPU, which leads to the buggy behaviour on better CPUs.
>> >> > It's far better to leave it as is and just accept that the old CPUs
>> >> > will have longer than necessary delays. If people really really
>> >> > care (and there's likely to only be a small minority of them now)
>> >> > changing the '0' to a '1' is a very simple change for them to carry
>> >> > in their local tree. Unlike getting the right unrolling etc.
>> >>
>> >> Well, they can also 'git revert' this patch. If somebody really cares
>> >> I think they should shout now and provide a better patch, otherwise
>> >> this one should be merged.
>> >
>> > On the other hand, having the code there as it currently stands is not
>> > harmful in any way, so leaving it there is just as easy.
>>
>> It makes the code less understandable. I'm not sure about linux's
>> practices, but an #if 0 generally means somebody is being lazy.
>
> I would agree with you if it was a complicated bit of code, but it
> isn't. It is a simple count to zero (or overflow) and terminate
> loop. And it's certainly not about me being lazy.
Maybe it's not complicated to you, but not everyone is so literate
about ARM assembly code (e.g. me). When I first looked at the code I
didn't even realize there was an #if 0 there, which yes, I grant is a
problem of my editor, but the issue wouldn't have happened if the code
wasn't there in the first place.
And I'm not saying your are being lazy, if anything it's probably the
people using this code. They should figure a way to avoid patching the
code. However, these users are hypothetical at this point.
> Unless there is a strong argument for removing it, the code stays as
> is.
>
> So far, the argument is basically "it's a #if 0, we must get rid of
> it" which is a religous argument, not a technical one. The fact is
> that (as I said above) keeping it there provides the code for when
> people want to enable it. That's a technical reason for keeping it.
>
> Please can we now move to something more productive instead of this
> religous argument?
I don't think a good argument has been stated, I'll try to do that on
Wolfram's reply.
--
Felipe Contreras
On Mon, Sep 14, 2009 at 6:36 PM, Wolfram Sang <[email protected]> wrote:
> On Mon, Sep 14, 2009 at 06:14:08PM +0300, Felipe Contreras wrote:
>> On Mon, Sep 14, 2009 at 5:40 PM, Russell King - ARM Linux
>> <[email protected]> wrote:
>> > On Mon, Sep 14, 2009 at 05:38:32PM +0300, Felipe Contreras wrote:
>> >> On Mon, Sep 14, 2009 at 5:00 PM, Russell King - ARM Linux
>> >> <[email protected]> wrote:
>> >> > On Mon, Sep 14, 2009 at 03:58:24PM +0300, Felipe Contreras wrote:
>> >> >> On Mon, Sep 14, 2009 at 11:10 AM, Russell King - ARM Linux
>> >> >> <[email protected]> wrote:
>> >> >> > On Mon, Sep 14, 2009 at 01:21:00AM +0100, Jamie Lokier wrote:
>> >> >> >> Russell King - ARM Linux wrote:
>> >> >> >> > On Sun, Sep 13, 2009 at 11:28:47PM +0200, Marek Vasut wrote:
>> >> >> >> > > > bhi __delay
>> >> >> >> > > > mov pc, lr
>> >> >> >> > > > ENDPROC(__udelay)
>> >> >> >> > > >
>> >> >> >> > > Hi
>> >> >> >> > >
>> >> >> >> > > why was this code there in the first place ?
>> >> >> >> >
>> >> >> >> > To make the delay loop more stable and predictable on older CPUs.
>> >> >> >>
>> >> >> >> So why has it been commented out, if it's needed for that?
>> >> >> >
>> >> >> > We moved on and it penalises later CPUs, leading to udelay providing
>> >> >> > shorter delays than requested.
>> >> >> >
>> >> >> > So the choice was either stable and predictable on older CPUs but
>> >> >> > buggy on newer CPUs, or correct on all CPUs but gives unnecessarily
>> >> >> > longer delays on older CPUs.
>> >> >>
>> >> >> Why not add an #ifdef CPU_V4 or whatever?
>> >> >
>> >> > Because then you get it whenever you configure for V4 as the lowest
>> >> > denominator CPU, which leads to the buggy behaviour on better CPUs.
>> >> > It's far better to leave it as is and just accept that the old CPUs
>> >> > will have longer than necessary delays. If people really really
>> >> > care (and there's likely to only be a small minority of them now)
>> >> > changing the '0' to a '1' is a very simple change for them to carry
>> >> > in their local tree. Unlike getting the right unrolling etc.
>> >>
>> >> Well, they can also 'git revert' this patch. If somebody really cares
>> >> I think they should shout now and provide a better patch, otherwise
>> >> this one should be merged.
>> >
>> > On the other hand, having the code there as it currently stands is not
>> > harmful in any way, so leaving it there is just as easy.
>>
>> It makes the code less understandable. I'm not sure about linux's
>> practices, but an #if 0 generally means somebody is being lazy.
>
> And what about a comment like "Try including the following code if ..."? I
> think I saw this somewhere else in the kernel and I'd be fine with that. So,
> while I generally agree that "#if 0" looks suspicious, there are a few cases
> for it, though they need documentation IMHO.
I agree. I don't think the current form is acceptable, at the very
least there should be a comment: enable the following code if ...
Then the next question arises: why not make it a configuration option?
To which the answer seems to be: because very very few people would
care about this.
Which would then be followed by: if we don't care enough about this
code in order to have a configuration option to actually use it; why
do we have it all?
The minority that would make use of this code is theoretical, and
these users by definition know they need this code, therefore, if the
code is not there, they'll notice, then, if the change is acceptable
by them, they will just revert the patch and not complain, if it's
unacceptable, then they will complain and the code can be
re-introduced. The issue will sort itself out.
If it turns out nobody complains about removing this code you would
have made the linux kernel 16 lines cleaner for free.
Cheers.
--
Felipe Contreras
> The minority that would make use of this code is theoretical, and
> these users by definition know they need this code, therefore, if the
> code is not there, they'll notice,
I'm not sure everyone knows in advance what is going on. I imagine the case
where something is inadeqaute, one starts looking at the code and _after_
reading the #if 0-block, understands the problem and realizes that the block is
needed in this rare case. So, it's like some kind of documentation. This is why
I'd still favour the comment; if there is a chance for such a patch, I'd make
it BTW.
> If it turns out nobody complains about removing this code you would
> have made the linux kernel 16 lines cleaner for free.
I'd think we lose a bit of information. (Yeah, it is in the history, but harder
to find there.)
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Dne Po 14. září 2009 17:50:15 Felipe Contreras napsal(a):
> On Mon, Sep 14, 2009 at 6:21 PM, Russell King - ARM Linux
>
> <[email protected]> wrote:
> > On Mon, Sep 14, 2009 at 06:14:08PM +0300, Felipe Contreras wrote:
> >> On Mon, Sep 14, 2009 at 5:40 PM, Russell King - ARM Linux
> >>
> >> <[email protected]> wrote:
> >> > On Mon, Sep 14, 2009 at 05:38:32PM +0300, Felipe Contreras wrote:
> >> >> On Mon, Sep 14, 2009 at 5:00 PM, Russell King - ARM Linux
> >> >>
> >> >> <[email protected]> wrote:
> >> >> > On Mon, Sep 14, 2009 at 03:58:24PM +0300, Felipe Contreras wrote:
> >> >> >> On Mon, Sep 14, 2009 at 11:10 AM, Russell King - ARM Linux
> >> >> >>
> >> >> >> <[email protected]> wrote:
> >> >> >> > On Mon, Sep 14, 2009 at 01:21:00AM +0100, Jamie Lokier wrote:
> >> >> >> >> Russell King - ARM Linux wrote:
> >> >> >> >> > On Sun, Sep 13, 2009 at 11:28:47PM +0200, Marek Vasut wrote:
> >> >> >> >> > > > bhi __delay
> >> >> >> >> > > > mov pc, lr
> >> >> >> >> > > > ENDPROC(__udelay)
> >> >> >> >> > >
> >> >> >> >> > > Hi
> >> >> >> >> > >
> >> >> >> >> > > why was this code there in the first place ?
> >> >> >> >> >
> >> >> >> >> > To make the delay loop more stable and predictable on older
> >> >> >> >> > CPUs.
> >> >> >> >>
> >> >> >> >> So why has it been commented out, if it's needed for that?
> >> >> >> >
> >> >> >> > We moved on and it penalises later CPUs, leading to udelay
> >> >> >> > providing shorter delays than requested.
> >> >> >> >
> >> >> >> > So the choice was either stable and predictable on older CPUs
> >> >> >> > but buggy on newer CPUs, or correct on all CPUs but gives
> >> >> >> > unnecessarily longer delays on older CPUs.
> >> >> >>
> >> >> >> Why not add an #ifdef CPU_V4 or whatever?
> >> >> >
> >> >> > Because then you get it whenever you configure for V4 as the lowest
> >> >> > denominator CPU, which leads to the buggy behaviour on better CPUs.
> >> >> > It's far better to leave it as is and just accept that the old CPUs
> >> >> > will have longer than necessary delays. If people really really
> >> >> > care (and there's likely to only be a small minority of them now)
> >> >> > changing the '0' to a '1' is a very simple change for them to carry
> >> >> > in their local tree. Unlike getting the right unrolling etc.
> >> >>
> >> >> Well, they can also 'git revert' this patch. If somebody really cares
> >> >> I think they should shout now and provide a better patch, otherwise
> >> >> this one should be merged.
> >> >
> >> > On the other hand, having the code there as it currently stands is not
> >> > harmful in any way, so leaving it there is just as easy.
> >>
> >> It makes the code less understandable. I'm not sure about linux's
> >> practices, but an #if 0 generally means somebody is being lazy.
> >
> > I would agree with you if it was a complicated bit of code, but it
> > isn't. It is a simple count to zero (or overflow) and terminate
> > loop. And it's certainly not about me being lazy.
Russell, what about adding a comment somewhere explaining why it's there? That'd
be a fine fix I think.
>
> Maybe it's not complicated to you, but not everyone is so literate
> about ARM assembly code (e.g. me). When I first looked at the code I
> didn't even realize there was an #if 0 there, which yes, I grant is a
> problem of my editor, but the issue wouldn't have happened if the code
> wasn't there in the first place.
This is still religious argument and or your problem -- fix your editor.
>
> And I'm not saying your are being lazy, if anything it's probably the
> people using this code. They should figure a way to avoid patching the
> code. However, these users are hypothetical at this point.
If you could invest your time into investigating how to make this configurable
instead of arguing about #if 0, it'd be awesome.
>
> > Unless there is a strong argument for removing it, the code stays as
> > is.
> >
> > So far, the argument is basically "it's a #if 0, we must get rid of
> > it" which is a religous argument, not a technical one. The fact is
> > that (as I said above) keeping it there provides the code for when
> > people want to enable it. That's a technical reason for keeping it.
> >
> > Please can we now move to something more productive instead of this
> > religous argument?
>
> I don't think a good argument has been stated, I'll try to do that on
> Wolfram's reply.
>
On Mon, Sep 14, 2009 at 7:25 PM, Marek Vasut <[email protected]> wrote:
> Dne Po 14. září 2009 17:50:15 Felipe Contreras napsal(a):
>> On Mon, Sep 14, 2009 at 6:21 PM, Russell King - ARM Linux
>>
>> <[email protected]> wrote:
>> > On Mon, Sep 14, 2009 at 06:14:08PM +0300, Felipe Contreras wrote:
>> >> On Mon, Sep 14, 2009 at 5:40 PM, Russell King - ARM Linux
>> >>
>> >> <[email protected]> wrote:
>> >> > On Mon, Sep 14, 2009 at 05:38:32PM +0300, Felipe Contreras wrote:
>> >> >> On Mon, Sep 14, 2009 at 5:00 PM, Russell King - ARM Linux
>> >> >>
>> >> >> <[email protected]> wrote:
>> >> >> > On Mon, Sep 14, 2009 at 03:58:24PM +0300, Felipe Contreras wrote:
>> >> >> >> On Mon, Sep 14, 2009 at 11:10 AM, Russell King - ARM Linux
>> >> >> >>
>> >> >> >> <[email protected]> wrote:
>> >> >> >> > On Mon, Sep 14, 2009 at 01:21:00AM +0100, Jamie Lokier wrote:
>> >> >> >> >> Russell King - ARM Linux wrote:
>> >> >> >> >> > On Sun, Sep 13, 2009 at 11:28:47PM +0200, Marek Vasut wrote:
>> >> >> >> >> > > > bhi __delay
>> >> >> >> >> > > > mov pc, lr
>> >> >> >> >> > > > ENDPROC(__udelay)
>> >> >> >> >> > >
>> >> >> >> >> > > Hi
>> >> >> >> >> > >
>> >> >> >> >> > > why was this code there in the first place ?
>> >> >> >> >> >
>> >> >> >> >> > To make the delay loop more stable and predictable on older
>> >> >> >> >> > CPUs.
>> >> >> >> >>
>> >> >> >> >> So why has it been commented out, if it's needed for that?
>> >> >> >> >
>> >> >> >> > We moved on and it penalises later CPUs, leading to udelay
>> >> >> >> > providing shorter delays than requested.
>> >> >> >> >
>> >> >> >> > So the choice was either stable and predictable on older CPUs
>> >> >> >> > but buggy on newer CPUs, or correct on all CPUs but gives
>> >> >> >> > unnecessarily longer delays on older CPUs.
>> >> >> >>
>> >> >> >> Why not add an #ifdef CPU_V4 or whatever?
>> >> >> >
>> >> >> > Because then you get it whenever you configure for V4 as the lowest
>> >> >> > denominator CPU, which leads to the buggy behaviour on better CPUs.
>> >> >> > It's far better to leave it as is and just accept that the old CPUs
>> >> >> > will have longer than necessary delays. If people really really
>> >> >> > care (and there's likely to only be a small minority of them now)
>> >> >> > changing the '0' to a '1' is a very simple change for them to carry
>> >> >> > in their local tree. Unlike getting the right unrolling etc.
>> >> >>
>> >> >> Well, they can also 'git revert' this patch. If somebody really cares
>> >> >> I think they should shout now and provide a better patch, otherwise
>> >> >> this one should be merged.
>> >> >
>> >> > On the other hand, having the code there as it currently stands is not
>> >> > harmful in any way, so leaving it there is just as easy.
>> >>
>> >> It makes the code less understandable. I'm not sure about linux's
>> >> practices, but an #if 0 generally means somebody is being lazy.
>> >
>> > I would agree with you if it was a complicated bit of code, but it
>> > isn't. It is a simple count to zero (or overflow) and terminate
>> > loop. And it's certainly not about me being lazy.
>
> Russell, what about adding a comment somewhere explaining why it's there? That'd
> be a fine fix I think.
>>
>> Maybe it's not complicated to you, but not everyone is so literate
>> about ARM assembly code (e.g. me). When I first looked at the code I
>> didn't even realize there was an #if 0 there, which yes, I grant is a
>> problem of my editor, but the issue wouldn't have happened if the code
>> wasn't there in the first place.
>
> This is still religious argument and or your problem -- fix your editor.
I'm an example user. I'm pretty sure there's other vim users out there
:) Since I'm not touching the 'asm' configurations I'm assuming many
other people will see the code as I do. Not my main argument, anyway.
>> And I'm not saying your are being lazy, if anything it's probably the
>> people using this code. They should figure a way to avoid patching the
>> code. However, these users are hypothetical at this point.
>
> If you could invest your time into investigating how to make this configurable
> instead of arguing about #if 0, it'd be awesome.
That's what I'm doing, so far I haven't heard any suggestions on how to do that.
--
Felipe Contreras
> >> > Because then you get it whenever you configure for V4 as the lowest
> >> > denominator CPU, which leads to the buggy behaviour on better CPUs.
> >> > It's far better to leave it as is and just accept that the old CPUs
> >> > will have longer than necessary delays. ?If people really really
> >> > care (and there's likely to only be a small minority of them now)
> >> > changing the '0' to a '1' is a very simple change for them to carry
> >> > in their local tree. ?Unlike getting the right unrolling etc.
> >>
> >> Well, they can also 'git revert' this patch. If somebody really cares
> >> I think they should shout now and provide a better patch, otherwise
> >> this one should be merged.
> >
> > On the other hand, having the code there as it currently stands is not
> > harmful in any way, so leaving it there is just as easy.
>
> It makes the code less understandable. I'm not sure about linux's
> practices, but an #if 0 generally means somebody is being lazy.
Not in this case, as you was explained to you. You may want to add
explaining comment above #if 0....
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Tue, Sep 15, 2009 at 1:37 PM, Pavel Machek <[email protected]> wrote:
>
>> >> > Because then you get it whenever you configure for V4 as the lowest
>> >> > denominator CPU, which leads to the buggy behaviour on better CPUs.
>> >> > It's far better to leave it as is and just accept that the old CPUs
>> >> > will have longer than necessary delays. If people really really
>> >> > care (and there's likely to only be a small minority of them now)
>> >> > changing the '0' to a '1' is a very simple change for them to carry
>> >> > in their local tree. Unlike getting the right unrolling etc.
>> >>
>> >> Well, they can also 'git revert' this patch. If somebody really cares
>> >> I think they should shout now and provide a better patch, otherwise
>> >> this one should be merged.
>> >
>> > On the other hand, having the code there as it currently stands is not
>> > harmful in any way, so leaving it there is just as easy.
>>
>> It makes the code less understandable. I'm not sure about linux's
>> practices, but an #if 0 generally means somebody is being lazy.
>
> Not in this case, as you was explained to you. You may want to add
> explaining comment above #if 0....
Yes, but I've no idea in which situations somebody might want to
enable that code. Old chips? Which old chips?
--
Felipe Contreras
On Tue 2009-09-15 13:47:01, Felipe Contreras wrote:
> On Tue, Sep 15, 2009 at 1:37 PM, Pavel Machek <[email protected]> wrote:
> >
> >> >> > Because then you get it whenever you configure for V4 as the lowest
> >> >> > denominator CPU, which leads to the buggy behaviour on better CPUs.
> >> >> > It's far better to leave it as is and just accept that the old CPUs
> >> >> > will have longer than necessary delays. ?If people really really
> >> >> > care (and there's likely to only be a small minority of them now)
> >> >> > changing the '0' to a '1' is a very simple change for them to carry
> >> >> > in their local tree. ?Unlike getting the right unrolling etc.
> >> >>
> >> >> Well, they can also 'git revert' this patch. If somebody really cares
> >> >> I think they should shout now and provide a better patch, otherwise
> >> >> this one should be merged.
> >> >
> >> > On the other hand, having the code there as it currently stands is not
> >> > harmful in any way, so leaving it there is just as easy.
> >>
> >> It makes the code less understandable. I'm not sure about linux's
> >> practices, but an #if 0 generally means somebody is being lazy.
> >
> > Not in this case, as you was explained to you. You may want to add
> > explaining comment above #if 0....
>
> Yes, but I've no idea in which situations somebody might want to
> enable that code. Old chips? Which old chips?
If you udelay() produces too long delays, as was explained in the thread.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Tue, Sep 15, 2009 at 1:49 PM, Pavel Machek <[email protected]> wrote:
> On Tue 2009-09-15 13:47:01, Felipe Contreras wrote:
>> On Tue, Sep 15, 2009 at 1:37 PM, Pavel Machek <[email protected]> wrote:
>> >
>> >> >> > Because then you get it whenever you configure for V4 as the lowest
>> >> >> > denominator CPU, which leads to the buggy behaviour on better CPUs.
>> >> >> > It's far better to leave it as is and just accept that the old CPUs
>> >> >> > will have longer than necessary delays. If people really really
>> >> >> > care (and there's likely to only be a small minority of them now)
>> >> >> > changing the '0' to a '1' is a very simple change for them to carry
>> >> >> > in their local tree. Unlike getting the right unrolling etc.
>> >> >>
>> >> >> Well, they can also 'git revert' this patch. If somebody really cares
>> >> >> I think they should shout now and provide a better patch, otherwise
>> >> >> this one should be merged.
>> >> >
>> >> > On the other hand, having the code there as it currently stands is not
>> >> > harmful in any way, so leaving it there is just as easy.
>> >>
>> >> It makes the code less understandable. I'm not sure about linux's
>> >> practices, but an #if 0 generally means somebody is being lazy.
>> >
>> > Not in this case, as you was explained to you. You may want to add
>> > explaining comment above #if 0....
>>
>> Yes, but I've no idea in which situations somebody might want to
>> enable that code. Old chips? Which old chips?
>
> If you udelay() produces too long delays, as was explained in the thread.
Yeah, on "older CPUs", and what constitutes an "older CPU" has not been defined.
--
Felipe Contreras
On Tue, 2009-09-15 at 12:37 +0200, Pavel Machek wrote:
> > >> > Because then you get it whenever you configure for V4 as the lowest
> > >> > denominator CPU, which leads to the buggy behaviour on better CPUs.
> > >> > It's far better to leave it as is and just accept that the old CPUs
> > >> > will have longer than necessary delays. If people really really
> > >> > care (and there's likely to only be a small minority of them now)
> > >> > changing the '0' to a '1' is a very simple change for them to carry
> > >> > in their local tree. Unlike getting the right unrolling etc.
> > >>
> > >> Well, they can also 'git revert' this patch. If somebody really cares
> > >> I think they should shout now and provide a better patch, otherwise
> > >> this one should be merged.
> > >
> > > On the other hand, having the code there as it currently stands is not
> > > harmful in any way, so leaving it there is just as easy.
> >
> > It makes the code less understandable. I'm not sure about linux's
> > practices, but an #if 0 generally means somebody is being lazy.
>
> Not in this case, as you was explained to you. You may want to add
> explaining comment above #if 0....
> Pavel
Perhaps we can document with something like..
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index aef63c8..ca8d535 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -813,6 +813,14 @@ config ARM_ERRATA_460075
ACTLR register. Note that setting specific bits in the ACTLR
register
may not be available in non-secure mode.
+config OLD_CPU_DELAY
+ depends on CPU_32v3 || CPU_32v4 || CPU_32v4T
+ bool "Accurate delays for older CPU"
+ def_bool n
+ help
+ Enable if observing longer than expected delays and need more
+ accurate delays on older CPUs.
+
endmenu
source "arch/arm/common/Kconfig"
diff --git a/arch/arm/lib/delay.S b/arch/arm/lib/delay.S
index 8d6a876..8b3fa63 100644
--- a/arch/arm/lib/delay.S
+++ b/arch/arm/lib/delay.S
@@ -42,7 +42,7 @@ ENTRY(__const_udelay) @ 0 <= r0 <= 0x7fffff06
@ Delay routine
ENTRY(__delay)
subs r0, r0, #1
-#if 0
+#if CONFIG_OLD_CPU_DELAY
movls pc, lr
subs r0, r0, #1
movls pc, lr
Dne Út 15. září 2009 14:29:21 Steve Chen napsal(a):
> On Tue, 2009-09-15 at 12:37 +0200, Pavel Machek wrote:
> > > >> > Because then you get it whenever you configure for V4 as the
> > > >> > lowest denominator CPU, which leads to the buggy behaviour on
> > > >> > better CPUs. It's far better to leave it as is and just accept
> > > >> > that the old CPUs will have longer than necessary delays. If
> > > >> > people really really care (and there's likely to only be a small
> > > >> > minority of them now) changing the '0' to a '1' is a very simple
> > > >> > change for them to carry in their local tree. Unlike getting the
> > > >> > right unrolling etc.
> > > >>
> > > >> Well, they can also 'git revert' this patch. If somebody really
> > > >> cares I think they should shout now and provide a better patch,
> > > >> otherwise this one should be merged.
> > > >
> > > > On the other hand, having the code there as it currently stands is
> > > > not harmful in any way, so leaving it there is just as easy.
> > >
> > > It makes the code less understandable. I'm not sure about linux's
> > > practices, but an #if 0 generally means somebody is being lazy.
> >
> > Not in this case, as you was explained to you. You may want to add
> > explaining comment above #if 0....
> > Pavel
>
> Perhaps we can document with something like..
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index aef63c8..ca8d535 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -813,6 +813,14 @@ config ARM_ERRATA_460075
> ACTLR register. Note that setting specific bits in the ACTLR
> register
> may not be available in non-secure mode.
>
> +config OLD_CPU_DELAY
> + depends on CPU_32v3 || CPU_32v4 || CPU_32v4T
> + bool "Accurate delays for older CPU"
> + def_bool n
> + help
> + Enable if observing longer than expected delays and need more
> + accurate delays on older CPUs.
> +
> endmenu
>
> source "arch/arm/common/Kconfig"
> diff --git a/arch/arm/lib/delay.S b/arch/arm/lib/delay.S
> index 8d6a876..8b3fa63 100644
> --- a/arch/arm/lib/delay.S
> +++ b/arch/arm/lib/delay.S
> @@ -42,7 +42,7 @@ ENTRY(__const_udelay) @ 0 <= r0 <= 0x7fffff06
> @ Delay routine
> ENTRY(__delay)
> subs r0, r0, #1
> -#if 0
> +#if CONFIG_OLD_CPU_DELAY
ifdef please
> movls pc, lr
> subs r0, r0, #1
> movls pc, lr
>
On Tue, 2009-09-15 at 15:41 +0200, Marek Vasut wrote:
> > -#if 0
> > +#if CONFIG_OLD_CPU_DELAY
>
> ifdef please
>
I assume this a vote in favor of considering this patch as an
alternative :) The updated patch is below. Since I'm only summarizing
the e-mail thread and put them into a patch (which is a less time
consuming process than continue reading and deleting e-mails on this
thread), contributors can add signed off before submitting to Russel.
Document #if 0 code block in delay.S and make it selectable for compile.
Signed-off-by: Steve Chen <[email protected]>
---
arch/arm/Kconfig | 8 ++++++++
arch/arm/lib/delay.S | 2 +-
2 files changed, 9 insertions(+), 1 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index aef63c8..ca8d535 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -813,6 +813,14 @@ config ARM_ERRATA_460075
ACTLR register. Note that setting specific bits in the ACTLR
register
may not be available in non-secure mode.
+config OLD_CPU_DELAY
+ depends on CPU_32v3 || CPU_32v4 || CPU_32v4T
+ bool "Accurate delays for older CPU"
+ def_bool n
+ help
+ Enable if observing longer than expected delays and need more
+ accurate delays on older CPUs.
+
endmenu
source "arch/arm/common/Kconfig"
diff --git a/arch/arm/lib/delay.S b/arch/arm/lib/delay.S
index 8d6a876..67e679b 100644
--- a/arch/arm/lib/delay.S
+++ b/arch/arm/lib/delay.S
@@ -42,7 +42,7 @@ ENTRY(__const_udelay) @ 0 <= r0 <= 0x7fffff06
@ Delay routine
ENTRY(__delay)
subs r0, r0, #1
-#if 0
+#ifdef CONFIG_OLD_CPU_DELAY
movls pc, lr
subs r0, r0, #1
movls pc, lr
On Tue, Sep 15, 2009 at 7:30 PM, Steve Chen <[email protected]> wrote:
> On Tue, 2009-09-15 at 15:41 +0200, Marek Vasut wrote:
>> > -#if 0
>> > +#if CONFIG_OLD_CPU_DELAY
>>
>> ifdef please
>>
> I assume this a vote in favor of considering this patch as an
> alternative :) The updated patch is below. Since I'm only summarizing
> the e-mail thread and put them into a patch (which is a less time
> consuming process than continue reading and deleting e-mails on this
> thread), contributors can add signed off before submitting to Russel.
>
> Document #if 0 code block in delay.S and make it selectable for compile.
Nice :)
A few nitpicks though:
> Signed-off-by: Steve Chen <[email protected]>
>
> ---
>
> arch/arm/Kconfig | 8 ++++++++
> arch/arm/lib/delay.S | 2 +-
> 2 files changed, 9 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index aef63c8..ca8d535 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -813,6 +813,14 @@ config ARM_ERRATA_460075
> ACTLR register. Note that setting specific bits in the ACTLR
> register
> may not be available in non-secure mode.
>
> +config OLD_CPU_DELAY
> + depends on CPU_32v3 || CPU_32v4 || CPU_32v4T
> + bool "Accurate delays for older CPU"
s/older CPU/old CPUs/
Or even better, since this option will only appear on older CPUs:
"Accurate delays"
> + def_bool n
> + help
> + Enable if observing longer than expected delays and need more
> + accurate delays on older CPUs.
How about:
> + Enable if observing longer than expected delays and need more
> + accuracy. This should only be considered for old CPUs (e.g. foo, bar).
Cheers.
--
Felipe Contreras
Felipe,
Here is the updated patch. If you don't mind add your signed-off, I can
submit the patch.
Regards,
Steve
Document #if 0 code block in delay.S and make it selectable for compile.
Signed-off-by: Steve Chen <[email protected]>
---
arch/arm/Kconfig | 8 ++++++++
arch/arm/lib/delay.S | 2 +-
2 files changed, 9 insertions(+), 1 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index aef63c8..ca8d535 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -813,6 +813,14 @@ config ARM_ERRATA_460075
ACTLR register. Note that setting specific bits in the ACTLR
register
may not be available in non-secure mode.
+config OLD_CPU_DELAY
+ depends on CPU_32v3 || CPU_32v4 || CPU_32v4T
+ bool "Accurate delays"
+ def_bool n
+ help
+ Enable if observing longer than expected delays and need more
+ accuracy. This only applies to older CPUs.
+
endmenu
source "arch/arm/common/Kconfig"
diff --git a/arch/arm/lib/delay.S b/arch/arm/lib/delay.S
index 8d6a876..67e679b 100644
--- a/arch/arm/lib/delay.S
+++ b/arch/arm/lib/delay.S
@@ -42,7 +42,7 @@ ENTRY(__const_udelay) @ 0 <= r0 <= 0x7fffff06
@ Delay routine
ENTRY(__delay)
subs r0, r0, #1
-#if 0
+#ifdef CONFIG_OLD_CPU_DELAY
movls pc, lr
subs r0, r0, #1
movls pc, lr
Dne Út 15. září 2009 21:44:35 Steve Chen napsal(a):
> Felipe,
>
> Here is the updated patch. If you don't mind add your signed-off, I can
> submit the patch.
Do you guys really have so much free time on your hands to argue about this?
btw. here's my nitpick then -- 'accurate delays' is really bad way to call it
;-)
>
> Regards,
>
> Steve
>
> Document #if 0 code block in delay.S and make it selectable for compile.
>
> Signed-off-by: Steve Chen <[email protected]>
>
> ---
>
> arch/arm/Kconfig | 8 ++++++++
> arch/arm/lib/delay.S | 2 +-
> 2 files changed, 9 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index aef63c8..ca8d535 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -813,6 +813,14 @@ config ARM_ERRATA_460075
> ACTLR register. Note that setting specific bits in the ACTLR
> register
> may not be available in non-secure mode.
>
> +config OLD_CPU_DELAY
> + depends on CPU_32v3 || CPU_32v4 || CPU_32v4T
> + bool "Accurate delays"
> + def_bool n
> + help
> + Enable if observing longer than expected delays and need more
> + accuracy. This only applies to older CPUs.
> +
> endmenu
>
> source "arch/arm/common/Kconfig"
> diff --git a/arch/arm/lib/delay.S b/arch/arm/lib/delay.S
> index 8d6a876..67e679b 100644
> --- a/arch/arm/lib/delay.S
> +++ b/arch/arm/lib/delay.S
> @@ -42,7 +42,7 @@ ENTRY(__const_udelay) @ 0 <= r0 <= 0x7fffff06
> @ Delay routine
> ENTRY(__delay)
> subs r0, r0, #1
> -#if 0
> +#ifdef CONFIG_OLD_CPU_DELAY
> movls pc, lr
> subs r0, r0, #1
> movls pc, lr
>
Steve Chen <[email protected]> writes:
> +config OLD_CPU_DELAY
> + depends on CPU_32v3 || CPU_32v4 || CPU_32v4T
> + bool "Accurate delays"
> + def_bool n
> + help
> + Enable if observing longer than expected delays and need more
> + accuracy. This only applies to older CPUs.
> +
If it's that simple then why not enable the extra delay code
unconditionally when compiling for those CPUs?
We don't need "do something better" options.
--
Krzysztof Halasa
On Tue, 2009-09-15 at 23:04 +0200, Krzysztof Halasa wrote:
> Steve Chen <[email protected]> writes:
>
> > +config OLD_CPU_DELAY
> > + depends on CPU_32v3 || CPU_32v4 || CPU_32v4T
> > + bool "Accurate delays"
> > + def_bool n
> > + help
> > + Enable if observing longer than expected delays and need more
> > + accuracy. This only applies to older CPUs.
> > +
>
> If it's that simple then why not enable the extra delay code
> unconditionally when compiling for those CPUs?
>
> We don't need "do something better" options.
If I know for sure that these are the processors that need that block of
code, your suggestion makes perfect sense. Unfortunately, I don't know.
My primary goals are
1. Put a stop to this thread (so far I have failed miserably)
2. Document that block of code.
3. If it makes someone's life easier.. it would be a bonus.
Steve
Steve Chen <[email protected]> writes:
> If I know for sure that these are the processors that need that block of
> code, your suggestion makes perfect sense. Unfortunately, I don't know.
Then I wouldn't write "Accurate delays". "Different", maybe.
"Accurate", we don't know.
> My primary goals are
>
> 1. Put a stop to this thread (so far I have failed miserably)
> 2. Document that block of code.
> 3. If it makes someone's life easier.. it would be a bonus.
Uncertain comments don't make life easier. Just write what's certain,
or nothing if nothing is.
--
Krzysztof Halasa
On Wed, 2009-09-16 at 00:47 +0200, Krzysztof Halasa wrote:
> Steve Chen <[email protected]> writes:
>
> > If I know for sure that these are the processors that need that block of
> > code, your suggestion makes perfect sense. Unfortunately, I don't know.
>
> Then I wouldn't write "Accurate delays". "Different", maybe.
> "Accurate", we don't know.
You are right. "Accurate delays" isn't appropriate. Probably want
something along the lines of "try this and see if helps".
>
> > My primary goals are
> >
> > 1. Put a stop to this thread (so far I have failed miserably)
> > 2. Document that block of code.
> > 3. If it makes someone's life easier.. it would be a bonus.
>
> Uncertain comments don't make life easier. Just write what's certain,
> or nothing if nothing is.
Makes sense. Based on both yours and Marek's inputs, I changed the
wording. The patch is below.
Document #if 0 code block in delay.S and make it selectable for compile.
Signed-off-by: Steve Chen <[email protected]>
---
arch/arm/Kconfig | 8 ++++++++
arch/arm/lib/delay.S | 2 +-
2 files changed, 9 insertions(+), 1 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index aef63c8..ca8d535 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -813,6 +813,14 @@ config ARM_ERRATA_460075
ACTLR register. Note that setting specific bits in the ACTLR
register
may not be available in non-secure mode.
+config OLD_CPU_DELAY
+ depends on CPU_32v3 || CPU_32v4 || CPU_32v4T
+ bool "Accurate delays for some older CPUs"
+ def_bool n
+ help
+ Try enable this if observing longer than expected delays and need
+ more accuracy. May cause instability in some CPUs.
+
endmenu
source "arch/arm/common/Kconfig"
diff --git a/arch/arm/lib/delay.S b/arch/arm/lib/delay.S
index 8d6a876..67e679b 100644
--- a/arch/arm/lib/delay.S
+++ b/arch/arm/lib/delay.S
@@ -42,7 +42,7 @@ ENTRY(__const_udelay) @ 0 <= r0 <= 0x7fffff06
@ Delay routine
ENTRY(__delay)
subs r0, r0, #1
-#if 0
+#ifdef CONFIG_OLD_CPU_DELAY
movls pc, lr
subs r0, r0, #1
movls pc, lr
Steve Chen <[email protected]> writes:
> +config OLD_CPU_DELAY
> + depends on CPU_32v3 || CPU_32v4 || CPU_32v4T
> + bool "Accurate delays for some older CPUs"
> + def_bool n
> + help
> + Try enable this if observing longer than expected delays and need
> + more accuracy. May cause instability in some CPUs.
> +
There is still this uncertain "accurate" in the text. Why don't just
+ bool "Different delay() code for some older CPUs"
--
Krzysztof Halasa
On Tue, Sep 15, 2009 at 11:04:37PM +0200, Krzysztof Halasa wrote:
> Steve Chen <[email protected]> writes:
>
> > +config OLD_CPU_DELAY
> > + depends on CPU_32v3 || CPU_32v4 || CPU_32v4T
> > + bool "Accurate delays"
> > + def_bool n
> > + help
> > + Enable if observing longer than expected delays and need more
> > + accuracy. This only applies to older CPUs.
> > +
>
> If it's that simple then why not enable the extra delay code
> unconditionally when compiling for those CPUs?
Because it's really not that clear cut. Eg, ARM610 and ARM710 work
better with it, but StrongARM suffers from delays being too short.
Having a kernel configured for all those processors used to be common
(since the Acorn RiscPC had pluggable CPU cards, which could be one
of those processors.)
It's really something that only experienced people should worry
about, and not Joe "kernel-builder" Bloggs.
On Wed, 2009-09-16 at 14:30 +0200, Krzysztof Halasa wrote:
> Steve Chen <[email protected]> writes:
>
> > +config OLD_CPU_DELAY
> > + depends on CPU_32v3 || CPU_32v4 || CPU_32v4T
> > + bool "Accurate delays for some older CPUs"
> > + def_bool n
> > + help
> > + Try enable this if observing longer than expected delays and need
> > + more accuracy. May cause instability in some CPUs.
> > +
>
> There is still this uncertain "accurate" in the text. Why don't just
>
> + bool "Different delay() code for some older CPUs"
Sounds good. I also update text under help to better quantify
"accuracy". Updated patch below. Separately, as Russel pointed out,
this should only be done by experienced people, so we even bother with
this patch since the only people should touch this are the people who
knows all the details.
Document #if 0 code block in delay.S and make it selectable for compile.
Signed-off-by: Steve Chen <[email protected]>
---
arch/arm/Kconfig | 8 ++++++++
arch/arm/lib/delay.S | 2 +-
2 files changed, 9 insertions(+), 1 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index aef63c8..ca8d535 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -813,6 +813,14 @@ config ARM_ERRATA_460075
ACTLR register. Note that setting specific bits in the ACTLR
register
may not be available in non-secure mode.
+config OLD_CPU_DELAY
+ depends on CPU_32v3 || CPU_32v4 || CPU_32v4T
+ bool "Different delay() code for some older CPUs"
+ def_bool n
+ help
+ Try enable this if observing longer than expected delays. This code
+ improves accuracy for some CPUs while cause instability in others.
+
endmenu
source "arch/arm/common/Kconfig"
diff --git a/arch/arm/lib/delay.S b/arch/arm/lib/delay.S
index 8d6a876..67e679b 100644
--- a/arch/arm/lib/delay.S
+++ b/arch/arm/lib/delay.S
@@ -42,7 +42,7 @@ ENTRY(__const_udelay) @ 0 <= r0 <= 0x7fffff06
@ Delay routine
ENTRY(__delay)
subs r0, r0, #1
-#if 0
+#ifdef CONFIG_OLD_CPU_DELAY
movls pc, lr
subs r0, r0, #1
movls pc, lr
Hello.
Steve Chen wrote:
>>> +config OLD_CPU_DELAY
>>> + depends on CPU_32v3 || CPU_32v4 || CPU_32v4T
>>> + bool "Accurate delays for some older CPUs"
>>> + def_bool n
>>> + help
>>> + Try enable this if observing longer than expected delays and need
>>> + more accuracy. May cause instability in some CPUs.
>>> +
>>>
>> There is still this uncertain "accurate" in the text. Why don't just
>>
>> + bool "Different delay() code for some older CPUs"
>>
>
> Sounds good. I also update text under help to better quantify
> "accuracy". Updated patch below. Separately, as Russel pointed out,
> this should only be done by experienced people, so we even bother with
> this patch since the only people should touch this are the people who
> knows all the details.
>
> Document #if 0 code block in delay.S and make it selectable for compile.
>
> Signed-off-by: Steve Chen <[email protected]>
>
> ---
>
> arch/arm/Kconfig | 8 ++++++++
> arch/arm/lib/delay.S | 2 +-
> 2 files changed, 9 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index aef63c8..ca8d535 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -813,6 +813,14 @@ config ARM_ERRATA_460075
> ACTLR register. Note that setting specific bits in the ACTLR
> register
> may not be available in non-secure mode.
>
> +config OLD_CPU_DELAY
> + depends on CPU_32v3 || CPU_32v4 || CPU_32v4T
> + bool "Different delay() code for some older CPUs"
> + def_bool n
> + help
> + Try enable this if observing longer than expected delays. This code
>
Only "enabling"... ;-)
WBR, Sergei
Russell King - ARM Linux wrote:
> On Tue, Sep 15, 2009 at 11:04:37PM +0200, Krzysztof Halasa wrote:
> > Steve Chen <[email protected]> writes:
> >
> > > +config OLD_CPU_DELAY
> > > + depends on CPU_32v3 || CPU_32v4 || CPU_32v4T
> > > + bool "Accurate delays"
> > > + def_bool n
> > > + help
> > > + Enable if observing longer than expected delays and need more
> > > + accuracy. This only applies to older CPUs.
> > > +
> >
> > If it's that simple then why not enable the extra delay code
> > unconditionally when compiling for those CPUs?
>
> Because it's really not that clear cut. Eg, ARM610 and ARM710 work
> better with it, but StrongARM suffers from delays being too short.
> Having a kernel configured for all those processors used to be common
> (since the Acorn RiscPC had pluggable CPU cards, which could be one
> of those processors.)
>
> It's really something that only experienced people should worry
> about, and not Joe "kernel-builder" Bloggs.
I'm confused now. If I'm building a "generic" kernel to run on
several different systems, including some ARM710s and some StrongARMs,
do I include the code or not?
Btw, do you know where PT110 fits in? Is it like StrongARM (SA110)?
Thanks,
-- Jamie
On Thu, Sep 17, 2009 at 08:32:57PM +0100, Jamie Lokier wrote:
> Russell King - ARM Linux wrote:
> > On Tue, Sep 15, 2009 at 11:04:37PM +0200, Krzysztof Halasa wrote:
> > > Steve Chen <[email protected]> writes:
> > >
> > > > +config OLD_CPU_DELAY
> > > > + depends on CPU_32v3 || CPU_32v4 || CPU_32v4T
> > > > + bool "Accurate delays"
> > > > + def_bool n
> > > > + help
> > > > + Enable if observing longer than expected delays and need more
> > > > + accuracy. This only applies to older CPUs.
> > > > +
> > >
> > > If it's that simple then why not enable the extra delay code
> > > unconditionally when compiling for those CPUs?
> >
> > Because it's really not that clear cut. Eg, ARM610 and ARM710 work
> > better with it, but StrongARM suffers from delays being too short.
> > Having a kernel configured for all those processors used to be common
> > (since the Acorn RiscPC had pluggable CPU cards, which could be one
> > of those processors.)
> >
> > It's really something that only experienced people should worry
> > about, and not Joe "kernel-builder" Bloggs.
>
> I'm confused now. If I'm building a "generic" kernel to run on
> several different systems, including some ARM710s and some StrongARMs,
> do I include the code or not?
You should not - because with the code enabled, StrongARM will violate
the guarantee that udelay() shall return after _at_ _least_ the delay
asked for.
However, for short delays, ARM710 will provide a delay unnecessarily in
excess of the requested delay - which conforms to the required guarantee
but is not desirable for some applications (eg, where you're using
udelay() to time hardware signals for clocking data.)
> Btw, do you know where PT110 fits in? Is it like StrongARM (SA110)?
No idea.
Document #if 0 code block in delay.S and make it selectable for compile.
Signed-off-by: Steve Chen <[email protected]>
---
arch/arm/Kconfig | 14 ++++++++++++++
arch/arm/lib/delay.S | 2 +-
2 files changed, 15 insertions(+), 1 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index aef63c8..ed8bf14 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -813,6 +813,20 @@ config ARM_ERRATA_460075
ACTLR register. Note that setting specific bits in the ACTLR
register
may not be available in non-secure mode.
+config OLD_CPU_DELAY
+ depends on CPU_32v3 || CPU_32v4 || CPU_32v4T
+ bool "Different delay() code for some older CPUs"
+ def_bool n
+ help
+ Enable this if observing longer than expected delays. This code
+ improves delay accuracy for some CPUs. However, it can also cause
+ delay duration to be too short for others which leads to stability
+ issues.
+
+ In other words, do not enable unless you can guarantee that the
+ processor (or ALL of the processors if building a generic kernel)
+ delays for at least the time requested after enabling.
+
endmenu
source "arch/arm/common/Kconfig"
diff --git a/arch/arm/lib/delay.S b/arch/arm/lib/delay.S
index 8d6a876..67e679b 100644
--- a/arch/arm/lib/delay.S
+++ b/arch/arm/lib/delay.S
@@ -42,7 +42,7 @@ ENTRY(__const_udelay) @ 0 <= r0 <= 0x7fffff06
@ Delay routine
ENTRY(__delay)
subs r0, r0, #1
-#if 0
+#ifdef CONFIG_OLD_CPU_DELAY
movls pc, lr
subs r0, r0, #1
movls pc, lr
Steve Chen wrote:
> + help
> + Enable this if observing longer than expected delays. This code
> + improves delay accuracy for some CPUs. However, it can also cause
> + delay duration to be too short for others which leads to stability
> + issues.
> +
> + In other words, do not enable unless you can guarantee that the
> + processor (or ALL of the processors if building a generic kernel)
> + delays for at least the time requested after enabling.
> +
> endmenu
It would be very helpful to mention which CPUs are affected.
Ok, we don't know exactly...
But Russell has stated that ARM610 and ARM710 benefit from enabling
the code, and that it specifically must not be enabled for StrongARM.
Here's an additional paragraph to add on the end:
ARM610 and ARM710 are known to benefit from enabling this option.
It should not be enabled for StrongARMs, because it is known
to produce too short delays on those.
Maybe you can adjust the config selectors and default to match?
-- Jamie
On Thu, 2009-09-17 at 22:37 +0100, Jamie Lokier wrote:
> Steve Chen wrote:
> > + help
> > + Enable this if observing longer than expected delays. This code
> > + improves delay accuracy for some CPUs. However, it can also cause
> > + delay duration to be too short for others which leads to stability
> > + issues.
> > +
> > + In other words, do not enable unless you can guarantee that the
> > + processor (or ALL of the processors if building a generic kernel)
> > + delays for at least the time requested after enabling.
> > +
> > endmenu
>
> It would be very helpful to mention which CPUs are affected.
> Ok, we don't know exactly...
>
> But Russell has stated that ARM610 and ARM710 benefit from enabling
> the code, and that it specifically must not be enabled for StrongARM.
>
> Here's an additional paragraph to add on the end:
>
> ARM610 and ARM710 are known to benefit from enabling this option.
> It should not be enabled for StrongARMs, because it is known
> to produce too short delays on those.
>
Sure, I can add this paragraph in the next revision.
> Maybe you can adjust the config selectors and default to match?
I'm not sure about this one. For one thing, in the last e-mail between
you and Russel I saw
>> Btw, do you know where PT110 fits in? Is it like StrongARM (SA110)?
> No idea.
Not to mention the difficulties to account for all the possible
permutations for kernels that is built for multiple CPUs. Perhaps we
can start a list of the processors that should have this flag enabled
and the list of processors should have the flag disabled (instead of the
paragraph you suggested). This way, people can just add to the list as
they experiment with this flag on whatever CPU version that they worked
on.
Regards,
Steve